mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process
@ 2021-06-18  8:18 Yonglong Li
  2021-06-18  8:18 ` [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18  8:18 UTC (permalink / raw)
  To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, Yonglong Li

fix issue: ADD_ADDR and RM_ADDR use pm.add_signal to mark event, so 
in some case pm.add_signal will be flush when ADD_ADDR/RM_ADDR in 
process.

fix issue: if ADD_ADDR and ADD_ADDR-echo process at the same time, 
only one event can write pm.add_signal. so ADD_ADDR will process 
after add_timer timeout or ADD_ADDR-echo will not be process.

Patch 1 fix ADD_ADDR and RM_ADDR maybe clear addr_signal each other.

Patch 2 and 3 deal ADD_ADDR and ADD_ADDR-echo with separately to fix 
conflicts in using pm.addr_signal porcess.

Patch 4 MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT is not necessary.

v1->v2:
 - remove READ_ONCE under the pm spin lock.

v2->v3:
 - Patch 2: rename mptcp_pm_should_add_addr to mptcp_pm_should_add_signal_addr
 - Patch 3: avoid read-modify-write of msk->pm.addr_signal and change  
   mptcp_pm_add_addr_signal to return void.

v3->v4:
 - Patch 1: use ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO)) instead 
   of BIT(MPTCP_RM_ADDR_SIGNAL)
 - Patch 3: simple the code; init flags; fix wrong goto logic code; 

Yonglong Li (4):
  mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
  mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
  mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT

 include/net/mptcp.h    |   1 +
 net/mptcp/options.c    | 161 ++++++++++++++++++++++++++++++++-----------------
 net/mptcp/pm.c         |  53 +++++++---------
 net/mptcp/pm_netlink.c |  10 ++-
 net/mptcp/protocol.h   |  31 ++++------
 5 files changed, 147 insertions(+), 109 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
  2021-06-18  8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
@ 2021-06-18  8:18 ` Yonglong Li
  2021-06-18  8:18 ` [PATCH v4 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18  8:18 UTC (permalink / raw)
  To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, Yonglong Li

ADD_ADDR share pm.addr_signal with RM_ADDR, so after RM_ADDR/ADD_ADDR
done we should not clean ADD_ADDR/RM_ADDR's addr_signal.

Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/pm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 9d00fa6..6c427c8 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -252,6 +252,7 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
 bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			      struct mptcp_addr_info *saddr, bool *echo, bool *port)
 {
+	u8 add_addr;
 	int ret = false;
 
 	spin_lock_bh(&msk->pm.lock);
@@ -267,7 +268,8 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 		goto out_unlock;
 
 	*saddr = msk->pm.local;
-	WRITE_ONCE(msk->pm.addr_signal, 0);
+	add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
+	WRITE_ONCE(msk->pm.addr_signal, add_addr);
 	ret = true;
 
 out_unlock:
@@ -278,6 +280,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list)
 {
+	u8 rm_addr;
 	int ret = false, len;
 
 	spin_lock_bh(&msk->pm.lock);
@@ -286,16 +289,17 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 	if (!mptcp_pm_should_rm_signal(msk))
 		goto out_unlock;
 
+	rm_addr = msk->pm.addr_signal & ~BIT(MPTCP_RM_ADDR_SIGNAL);
 	len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
 	if (len < 0) {
-		WRITE_ONCE(msk->pm.addr_signal, 0);
+		WRITE_ONCE(msk->pm.addr_signal, rm_addr);
 		goto out_unlock;
 	}
 	if (remaining < len)
 		goto out_unlock;
 
 	*rm_list = msk->pm.rm_list_tx;
-	WRITE_ONCE(msk->pm.addr_signal, 0);
+	WRITE_ONCE(msk->pm.addr_signal, rm_addr);
 	ret = true;
 
 out_unlock:
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
  2021-06-18  8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
  2021-06-18  8:18 ` [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
@ 2021-06-18  8:18 ` Yonglong Li
  2021-06-18  8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
  2021-06-18  8:18 ` [PATCH v4 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
  3 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18  8:18 UTC (permalink / raw)
  To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, Yonglong Li

MPTCP_ADD_ADDR_SIGNAL only for action of sending ADD_ADDR
MPTCP_ADD_ADDR_ECHO only for action of sending echo ADD_ADDR
add a mptcp_addr_info in struct mptcp_out_options for echo ADD_ADDR

to prepare for the next patch.

Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 include/net/mptcp.h    |  1 +
 net/mptcp/pm.c         | 13 ++++++++-----
 net/mptcp/pm_netlink.c |  4 ++--
 net/mptcp/protocol.h   |  6 ++++++
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index d61bbbf..637e90b 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -62,6 +62,7 @@ struct mptcp_out_options {
 	u64 rcvr_key;
 	u64 ahmac;
 	struct mptcp_addr_info addr;
+	struct mptcp_addr_info remote;
 	struct mptcp_rm_list rm_list;
 	u8 join_id;
 	u8 backup;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6c427c8..107a5a2 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -18,7 +18,7 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 {
 	u8 add_addr = READ_ONCE(msk->pm.addr_signal);
 
-	pr_debug("msk=%p, local_id=%d", msk, addr->id);
+	pr_debug("msk=%p, local_id=%d, echo:%d", msk, addr->id, echo);
 
 	lockdep_assert_held(&msk->pm.lock);
 
@@ -27,10 +27,13 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 		return -EINVAL;
 	}
 
-	msk->pm.local = *addr;
-	add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL);
-	if (echo)
+	if (echo) {
+		msk->pm.remote = *addr;
 		add_addr |= BIT(MPTCP_ADD_ADDR_ECHO);
+	} else {
+		msk->pm.local = *addr;
+		add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL);
+	}
 	if (addr->family == AF_INET6)
 		add_addr |= BIT(MPTCP_ADD_ADDR_IPV6);
 	if (addr->port)
@@ -214,7 +217,7 @@ void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,
 
 void mptcp_pm_add_addr_send_ack(struct mptcp_sock *msk)
 {
-	if (!mptcp_pm_should_add_signal(msk))
+	if (!mptcp_pm_should_add_signal_echo(msk))
 		return;
 
 	mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d4732a4..0f302d2 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -317,14 +317,14 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
 	if (!entry->addr.id)
 		return;
 
-	if (mptcp_pm_should_add_signal(msk)) {
+	if (mptcp_pm_should_add_signal_addr(msk)) {
 		sk_reset_timer(sk, timer, jiffies + TCP_RTO_MAX / 8);
 		goto out;
 	}
 
 	spin_lock_bh(&msk->pm.lock);
 
-	if (!mptcp_pm_should_add_signal(msk)) {
+	if (!mptcp_pm_should_add_signal_addr(msk)) {
 		pr_debug("retransmit ADD_ADDR id=%d", entry->addr.id);
 		mptcp_pm_announce_addr(msk, &entry->addr, false);
 		mptcp_pm_add_addr_send_ack(msk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 160c2ab..a0b0ec0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -708,6 +708,12 @@ void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk,
 
 static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)
 {
+	return READ_ONCE(msk->pm.addr_signal) &
+		(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
+}
+
+static inline bool mptcp_pm_should_add_signal_addr(struct mptcp_sock *msk)
+{
 	return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_ADD_ADDR_SIGNAL);
 }
 
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-18  8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
  2021-06-18  8:18 ` [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
  2021-06-18  8:18 ` [PATCH v4 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
@ 2021-06-18  8:18 ` Yonglong Li
  2021-06-18 11:20   ` Geliang Tang
                     ` (2 more replies)
  2021-06-18  8:18 ` [PATCH v4 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
  3 siblings, 3 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18  8:18 UTC (permalink / raw)
  To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, Yonglong Li

according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
ADD_ADDR/echo-ADD_ADDR option

add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option

Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
 net/mptcp/pm.c       |  30 ++++---------
 net/mptcp/protocol.h |  13 +++---
 3 files changed, 92 insertions(+), 75 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 1aec016..43e3241 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
 	bool drop_other_suboptions = false;
 	unsigned int opt_size = *size;
-	bool echo;
-	bool port;
+	struct mptcp_addr_info remote;
+	struct mptcp_addr_info local;
+	u8 add_addr, flags = 0xff;
 	int len;
 
-	if ((mptcp_pm_should_add_signal_ipv6(msk) ||
-	     mptcp_pm_should_add_signal_port(msk) ||
-	     mptcp_pm_should_add_signal_echo(msk)) &&
-	    skb && skb_is_tcp_pure_ack(skb)) {
-		pr_debug("drop other suboptions");
-		opts->suboptions = 0;
-		opts->ext_copy.use_ack = 0;
-		opts->ext_copy.use_map = 0;
-		remaining += opt_size;
-		drop_other_suboptions = true;
-	}
-
-	if (!mptcp_pm_should_add_signal(msk) ||
-	    !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
-		return false;
-
-	len = mptcp_add_addr_len(opts->addr.family, echo, port);
-	if (remaining < len)
+	if (!mptcp_pm_should_add_signal(msk))
 		return false;
 
-	*size = len;
-	if (drop_other_suboptions)
-		*size -= opt_size;
-	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
-	if (!echo) {
+	*size = 0;
+	mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
+	if (mptcp_pm_should_add_signal_echo(msk)) {
+		if (skb && skb_is_tcp_pure_ack(skb)) {
+			pr_debug("drop other suboptions");
+			opts->suboptions = 0;
+			opts->ext_copy.use_ack = 0;
+			opts->ext_copy.use_map = 0;
+			remaining += opt_size;
+			drop_other_suboptions = true;
+		}
+		len = mptcp_add_addr_len(remote.family, true, !!remote.port);
+		if (remaining < len)
+			return false;
+		remaining -= len;
+		*size += len;
+		opts->remote = remote;
+		flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
+		opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
+		pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
+			 opts->remote.id, ntohs(opts->remote.port), add_addr);
+	} else if (mptcp_pm_should_add_signal_addr(msk)) {
+		if ((local.family == AF_INET6 || local.port) && skb &&
+		    skb_is_tcp_pure_ack(skb)) {
+			pr_debug("drop other suboptions");
+			opts->suboptions = 0;
+			opts->ext_copy.use_ack = 0;
+			opts->ext_copy.use_map = 0;
+			remaining += opt_size;
+			drop_other_suboptions = true;
+		}
+		len = mptcp_add_addr_len(local.family, false, !!local.port);
+		if (remaining < len)
+			return false;
+		*size += len;
+		opts->addr = local;
 		opts->ahmac = add_addr_generate_hmac(msk->local_key,
 						     msk->remote_key,
 						     &opts->addr);
+		opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
+		flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
+		pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
+			 opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
 	}
-	pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
-		 opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
+
+	if (drop_other_suboptions)
+		*size -= opt_size;
+	spin_lock_bh(&msk->pm.lock);
+	WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
+	spin_unlock_bh(&msk->pm.lock);
 
 	return true;
 }
@@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 	}
 
 mp_capable_done:
-	if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
-		u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
-		u8 echo = MPTCP_ADDR_ECHO;
+	if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
+		struct mptcp_addr_info *addr_info;
+		u8 len = 0;
+		u8 echo = 0;
+
+		if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
+			len += sizeof(opts->ahmac);
+			addr_info = &opts->addr;
+		} else {
+			echo = MPTCP_ADDR_ECHO;
+			addr_info = &opts->remote;
+		}
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-		if (opts->addr.family == AF_INET6)
-			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+		if (addr_info->family == AF_INET6)
+			len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+		else
 #endif
+			len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
 
-		if (opts->addr.port)
+		if (addr_info->port)
 			len += TCPOLEN_MPTCP_PORT_LEN;
 
-		if (opts->ahmac) {
-			len += sizeof(opts->ahmac);
-			echo = 0;
-		}
-
 		*ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
-				      len, echo, opts->addr.id);
-		if (opts->addr.family == AF_INET) {
-			memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
+				      len, echo, addr_info->id);
+		if (addr_info->family == AF_INET) {
+			memcpy((u8 *)ptr, (u8 *)&addr_info->addr.s_addr, 4);
 			ptr += 1;
 		}
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-		else if (opts->addr.family == AF_INET6) {
-			memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
+		else if (addr_info->family == AF_INET6) {
+			memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
 			ptr += 4;
 		}
 #endif
 
-		if (!opts->addr.port) {
-			if (opts->ahmac) {
+		if (!addr_info->port) {
+			if (!echo) {
 				put_unaligned_be64(opts->ahmac, ptr);
 				ptr += 2;
 			}
 		} else {
-			u16 port = ntohs(opts->addr.port);
+			u16 port = ntohs(addr_info->port);
 
-			if (opts->ahmac) {
+			if (!echo) {
 				u8 *bptr = (u8 *)ptr;
 
 				put_unaligned_be16(port, bptr);
@@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 				bptr += 8;
 				put_unaligned_be16(TCPOPT_NOP << 8 |
 						   TCPOPT_NOP, bptr);
-
 				ptr += 3;
 			} else {
 				put_unaligned_be32(port << 16 |
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 107a5a2..a62d4a5 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 
 	lockdep_assert_held(&msk->pm.lock);
 
-	if (add_addr) {
+	if (add_addr &
+	    (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
 		pr_warn("addr_signal error, add_addr=%d", add_addr);
 		return -EINVAL;
 	}
@@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
 
 /* path manager helpers */
 
-bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
-			      struct mptcp_addr_info *saddr, bool *echo, bool *port)
+void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
+			      struct mptcp_addr_info *daddr, u8 *add_addr)
 {
-	u8 add_addr;
-	int ret = false;
-
 	spin_lock_bh(&msk->pm.lock);
 
-	/* double check after the lock is acquired */
-	if (!mptcp_pm_should_add_signal(msk))
-		goto out_unlock;
-
-	*echo = mptcp_pm_should_add_signal_echo(msk);
-	*port = mptcp_pm_should_add_signal_port(msk);
-
-	if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
-		goto out_unlock;
-
 	*saddr = msk->pm.local;
-	add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
-	WRITE_ONCE(msk->pm.addr_signal, add_addr);
-	ret = true;
+	*daddr = msk->pm.remote;
+	*add_addr = msk->pm.addr_signal;
 
-out_unlock:
 	spin_unlock_bh(&msk->pm.lock);
-	return ret;
+
+	if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
+		mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
 }
 
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index a0b0ec0..90fb532 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -22,10 +22,11 @@
 #define OPTION_MPTCP_MPJ_SYNACK	BIT(4)
 #define OPTION_MPTCP_MPJ_ACK	BIT(5)
 #define OPTION_MPTCP_ADD_ADDR	BIT(6)
-#define OPTION_MPTCP_RM_ADDR	BIT(7)
-#define OPTION_MPTCP_FASTCLOSE	BIT(8)
-#define OPTION_MPTCP_PRIO	BIT(9)
-#define OPTION_MPTCP_RST	BIT(10)
+#define OPTION_MPTCP_ADD_ECHO	BIT(7)
+#define OPTION_MPTCP_RM_ADDR	BIT(8)
+#define OPTION_MPTCP_FASTCLOSE	BIT(9)
+#define OPTION_MPTCP_PRIO	BIT(10)
+#define OPTION_MPTCP_RST	BIT(11)
 
 /* MPTCP option subtypes */
 #define MPTCPOPT_MP_CAPABLE	0
@@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
 	return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
 }
 
-bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
-			      struct mptcp_addr_info *saddr, bool *echo, bool *port);
+void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
+			      struct mptcp_addr_info *daddr, u8 *add_addr);
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
  2021-06-18  8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
                   ` (2 preceding siblings ...)
  2021-06-18  8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-18  8:18 ` Yonglong Li
  3 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18  8:18 UTC (permalink / raw)
  To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, Yonglong Li

there not need MPTCP_ADD_ADDR_PORT and MPTCP_ADD_ADDR_PORT, we can
get these info from pm.addr or pm.remote

Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/pm.c         |  4 ----
 net/mptcp/pm_netlink.c |  6 ++----
 net/mptcp/protocol.h   | 12 ------------
 3 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index a62d4a5..f051e48 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -35,10 +35,6 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 		msk->pm.local = *addr;
 		add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL);
 	}
-	if (addr->family == AF_INET6)
-		add_addr |= BIT(MPTCP_ADD_ADDR_IPV6);
-	if (addr->port)
-		add_addr |= BIT(MPTCP_ADD_ADDR_PORT);
 	WRITE_ONCE(msk->pm.addr_signal, add_addr);
 	return 0;
 }
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 0f302d2..bfa9d6d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -543,10 +543,8 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
 		bool slow;
 
 		spin_unlock_bh(&msk->pm.lock);
-		pr_debug("send ack for %s%s%s",
-			 mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr",
-			 mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
-			 mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
+		pr_debug("send ack for %s",
+			 mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
 
 		slow = lock_sock_fast(ssk);
 		tcp_send_ack(ssk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 90fb532..71e747c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -176,8 +176,6 @@ enum mptcp_pm_status {
 enum mptcp_addr_signal_status {
 	MPTCP_ADD_ADDR_SIGNAL,
 	MPTCP_ADD_ADDR_ECHO,
-	MPTCP_ADD_ADDR_IPV6,
-	MPTCP_ADD_ADDR_PORT,
 	MPTCP_RM_ADDR_SIGNAL,
 };
 
@@ -723,16 +721,6 @@ static inline bool mptcp_pm_should_add_signal_echo(struct mptcp_sock *msk)
 	return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_ADD_ADDR_ECHO);
 }
 
-static inline bool mptcp_pm_should_add_signal_ipv6(struct mptcp_sock *msk)
-{
-	return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_ADD_ADDR_IPV6);
-}
-
-static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk)
-{
-	return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_ADD_ADDR_PORT);
-}
-
 static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
 {
 	return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-18  8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-18 11:20   ` Geliang Tang
  2021-06-21  3:51     ` Yonglong Li
  2021-06-21  7:42   ` Geliang Tang
  2021-06-21  8:29   ` Geliang Tang
  2 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-18 11:20 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng

Hi Yonglong,

Thanks for v4!

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
>  net/mptcp/pm.c       |  30 ++++---------
>  net/mptcp/protocol.h |  13 +++---
>  3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>         bool drop_other_suboptions = false;
>         unsigned int opt_size = *size;
> -       bool echo;
> -       bool port;
> +       struct mptcp_addr_info remote;
> +       struct mptcp_addr_info local;
> +       u8 add_addr, flags = 0xff;
>         int len;
>
> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> -            mptcp_pm_should_add_signal_port(msk) ||
> -            mptcp_pm_should_add_signal_echo(msk)) &&
> -           skb && skb_is_tcp_pure_ack(skb)) {
> -               pr_debug("drop other suboptions");
> -               opts->suboptions = 0;
> -               opts->ext_copy.use_ack = 0;
> -               opts->ext_copy.use_map = 0;
> -               remaining += opt_size;
> -               drop_other_suboptions = true;
> -       }
> -
> -       if (!mptcp_pm_should_add_signal(msk) ||
> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> -               return false;
> -
> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> -       if (remaining < len)
> +       if (!mptcp_pm_should_add_signal(msk))
>                 return false;
>
> -       *size = len;
> -       if (drop_other_suboptions)
> -               *size -= opt_size;
> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> -       if (!echo) {
> +       *size = 0;
> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> +               if (skb && skb_is_tcp_pure_ack(skb)) {

'''
> +                       pr_debug("drop other suboptions");
> +                       opts->suboptions = 0;
> +                       opts->ext_copy.use_ack = 0;
> +                       opts->ext_copy.use_map = 0;
> +                       remaining += opt_size;
> +                       drop_other_suboptions = true;
'''

> +               }
> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> +               if (remaining < len)
> +                       return false;
> +               remaining -= len;
> +               *size += len;
> +               opts->remote = remote;
> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {
> +               if ((local.family == AF_INET6 || local.port) && skb &&
> +                   skb_is_tcp_pure_ack(skb)) {

'''
> +                       pr_debug("drop other suboptions");
> +                       opts->suboptions = 0;
> +                       opts->ext_copy.use_ack = 0;
> +                       opts->ext_copy.use_map = 0;
> +                       remaining += opt_size;
> +                       drop_other_suboptions = true;
'''

I think this "drop other suboptions" trunk here is still duplicated. Can
we just use one "drop other suboptions" trunk only?

Thanks.
-Geliang



> +               }
> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
> +               if (remaining < len)
> +                       return false;
> +               *size += len;
> +               opts->addr = local;
>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>                                                      msk->remote_key,
>                                                      &opts->addr);
> +               opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> +               pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> +                        opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
>         }
> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> +       if (drop_other_suboptions)
> +               *size -= opt_size;
> +       spin_lock_bh(&msk->pm.lock);
> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> +       spin_unlock_bh(&msk->pm.lock);
>
>         return true;
>  }
> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>         }
>
>  mp_capable_done:
> -       if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> -               u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> -               u8 echo = MPTCP_ADDR_ECHO;
> +       if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> +               struct mptcp_addr_info *addr_info;
> +               u8 len = 0;
> +               u8 echo = 0;
> +
> +               if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> +                       len += sizeof(opts->ahmac);
> +                       addr_info = &opts->addr;
> +               } else {
> +                       echo = MPTCP_ADDR_ECHO;
> +                       addr_info = &opts->remote;
> +               }
>
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -               if (opts->addr.family == AF_INET6)
> -                       len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +               if (addr_info->family == AF_INET6)
> +                       len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +               else
>  #endif
> +                       len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> -               if (opts->addr.port)
> +               if (addr_info->port)
>                         len += TCPOLEN_MPTCP_PORT_LEN;
>
> -               if (opts->ahmac) {
> -                       len += sizeof(opts->ahmac);
> -                       echo = 0;
> -               }
> -
>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> -                                     len, echo, opts->addr.id);
> -               if (opts->addr.family == AF_INET) {
> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> +                                     len, echo, addr_info->id);
> +               if (addr_info->family == AF_INET) {
> +                       memcpy((u8 *)ptr, (u8 *)&addr_info->addr.s_addr, 4);
>                         ptr += 1;
>                 }
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -               else if (opts->addr.family == AF_INET6) {
> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> +               else if (addr_info->family == AF_INET6) {
> +                       memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
>                         ptr += 4;
>                 }
>  #endif
>
> -               if (!opts->addr.port) {
> -                       if (opts->ahmac) {
> +               if (!addr_info->port) {
> +                       if (!echo) {
>                                 put_unaligned_be64(opts->ahmac, ptr);
>                                 ptr += 2;
>                         }
>                 } else {
> -                       u16 port = ntohs(opts->addr.port);
> +                       u16 port = ntohs(addr_info->port);
>
> -                       if (opts->ahmac) {
> +                       if (!echo) {
>                                 u8 *bptr = (u8 *)ptr;
>
>                                 put_unaligned_be16(port, bptr);
> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>                                 bptr += 8;
>                                 put_unaligned_be16(TCPOPT_NOP << 8 |
>                                                    TCPOPT_NOP, bptr);
> -
>                                 ptr += 3;
>                         } else {
>                                 put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..a62d4a5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
>         lockdep_assert_held(&msk->pm.lock);
>
> -       if (add_addr) {
> +       if (add_addr &
> +           (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>                 pr_warn("addr_signal error, add_addr=%d", add_addr);
>                 return -EINVAL;
>         }
> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
>  /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> +                             struct mptcp_addr_info *daddr, u8 *add_addr)
>  {
> -       u8 add_addr;
> -       int ret = false;
> -
>         spin_lock_bh(&msk->pm.lock);
>
> -       /* double check after the lock is acquired */
> -       if (!mptcp_pm_should_add_signal(msk))
> -               goto out_unlock;
> -
> -       *echo = mptcp_pm_should_add_signal_echo(msk);
> -       *port = mptcp_pm_should_add_signal_port(msk);
> -
> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> -               goto out_unlock;
> -
>         *saddr = msk->pm.local;
> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
> -       ret = true;
> +       *daddr = msk->pm.remote;
> +       *add_addr = msk->pm.addr_signal;
>
> -out_unlock:
>         spin_unlock_bh(&msk->pm.lock);
> -       return ret;
> +
> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>  }
>
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0b0ec0..90fb532 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -22,10 +22,11 @@
>  #define OPTION_MPTCP_MPJ_SYNACK        BIT(4)
>  #define OPTION_MPTCP_MPJ_ACK   BIT(5)
>  #define OPTION_MPTCP_ADD_ADDR  BIT(6)
> -#define OPTION_MPTCP_RM_ADDR   BIT(7)
> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> -#define OPTION_MPTCP_PRIO      BIT(9)
> -#define OPTION_MPTCP_RST       BIT(10)
> +#define OPTION_MPTCP_ADD_ECHO  BIT(7)
> +#define OPTION_MPTCP_RM_ADDR   BIT(8)
> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> +#define OPTION_MPTCP_PRIO      BIT(10)
> +#define OPTION_MPTCP_RST       BIT(11)
>
>  /* MPTCP option subtypes */
>  #define MPTCPOPT_MP_CAPABLE    0
> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>  }
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> +                             struct mptcp_addr_info *daddr, u8 *add_addr);
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>                              struct mptcp_rm_list *rm_list);
>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> --
> 1.8.3.1
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-18 11:20   ` Geliang Tang
@ 2021-06-21  3:51     ` Yonglong Li
  2021-06-21  6:42       ` Geliang Tang
  0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21  3:51 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng



On 2021/6/18 19:20, Geliang Tang wrote:
> Hi Yonglong,
> 
> Thanks for v4!
> 
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
>>
>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>> ADD_ADDR/echo-ADD_ADDR option
>>
>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>
>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>> ---
>>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
>>  net/mptcp/pm.c       |  30 ++++---------
>>  net/mptcp/protocol.h |  13 +++---
>>  3 files changed, 92 insertions(+), 75 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 1aec016..43e3241 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>         bool drop_other_suboptions = false;
>>         unsigned int opt_size = *size;
>> -       bool echo;
>> -       bool port;
>> +       struct mptcp_addr_info remote;
>> +       struct mptcp_addr_info local;
>> +       u8 add_addr, flags = 0xff;
>>         int len;
>>
>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>> -            mptcp_pm_should_add_signal_port(msk) ||
>> -            mptcp_pm_should_add_signal_echo(msk)) &&
>> -           skb && skb_is_tcp_pure_ack(skb)) {
>> -               pr_debug("drop other suboptions");
>> -               opts->suboptions = 0;
>> -               opts->ext_copy.use_ack = 0;
>> -               opts->ext_copy.use_map = 0;
>> -               remaining += opt_size;
>> -               drop_other_suboptions = true;
>> -       }
>> -
>> -       if (!mptcp_pm_should_add_signal(msk) ||
>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>> -               return false;
>> -
>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
>> -       if (remaining < len)
>> +       if (!mptcp_pm_should_add_signal(msk))
>>                 return false;
>>
>> -       *size = len;
>> -       if (drop_other_suboptions)
>> -               *size -= opt_size;
>> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> -       if (!echo) {
>> +       *size = 0;
>> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
>> +               if (skb && skb_is_tcp_pure_ack(skb)) {
> 
> '''
>> +                       pr_debug("drop other suboptions");
>> +                       opts->suboptions = 0;
>> +                       opts->ext_copy.use_ack = 0;
>> +                       opts->ext_copy.use_map = 0;
>> +                       remaining += opt_size;
>> +                       drop_other_suboptions = true;
> '''
> 
>> +               }
>> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>> +               if (remaining < len)
>> +                       return false;
>> +               remaining -= len;
>> +               *size += len;
>> +               opts->remote = remote;
>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
>> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {
>> +               if ((local.family == AF_INET6 || local.port) && skb &&
>> +                   skb_is_tcp_pure_ack(skb)) {
> 
> '''
>> +                       pr_debug("drop other suboptions");
>> +                       opts->suboptions = 0;
>> +                       opts->ext_copy.use_ack = 0;
>> +                       opts->ext_copy.use_map = 0;
>> +                       remaining += opt_size;
>> +                       drop_other_suboptions = true;
> '''
> 
> I think this "drop other suboptions" trunk here is still duplicated. Can
> we just use one "drop other suboptions" trunk only?
> 
> Thanks.
> -Geliang
> 
Hi Geliang, Thanks for you replay.

The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
IP version."
ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
to decide "drop other suboptions" in two trunk.

> 
> 
>> +               }
>> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
>> +               if (remaining < len)
>> +                       return false;
>> +               *size += len;
>> +               opts->addr = local;
>>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>                                                      msk->remote_key,
>>                                                      &opts->addr);
>> +               opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> +               pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
>> +                        opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
>>         }
>> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>> +
>> +       if (drop_other_suboptions)
>> +               *size -= opt_size;
>> +       spin_lock_bh(&msk->pm.lock);
>> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>> +       spin_unlock_bh(&msk->pm.lock);
>>
>>         return true;
>>  }
>> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>         }
>>
>>  mp_capable_done:
>> -       if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>> -               u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>> -               u8 echo = MPTCP_ADDR_ECHO;
>> +       if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
>> +               struct mptcp_addr_info *addr_info;
>> +               u8 len = 0;
>> +               u8 echo = 0;
>> +
>> +               if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>> +                       len += sizeof(opts->ahmac);
>> +                       addr_info = &opts->addr;
>> +               } else {
>> +                       echo = MPTCP_ADDR_ECHO;
>> +                       addr_info = &opts->remote;
>> +               }
>>
>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> -               if (opts->addr.family == AF_INET6)
>> -                       len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> +               if (addr_info->family == AF_INET6)
>> +                       len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> +               else
>>  #endif
>> +                       len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>
>> -               if (opts->addr.port)
>> +               if (addr_info->port)
>>                         len += TCPOLEN_MPTCP_PORT_LEN;
>>
>> -               if (opts->ahmac) {
>> -                       len += sizeof(opts->ahmac);
>> -                       echo = 0;
>> -               }
>> -
>>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>> -                                     len, echo, opts->addr.id);
>> -               if (opts->addr.family == AF_INET) {
>> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>> +                                     len, echo, addr_info->id);
>> +               if (addr_info->family == AF_INET) {
>> +                       memcpy((u8 *)ptr, (u8 *)&addr_info->addr.s_addr, 4);
>>                         ptr += 1;
>>                 }
>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> -               else if (opts->addr.family == AF_INET6) {
>> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>> +               else if (addr_info->family == AF_INET6) {
>> +                       memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
>>                         ptr += 4;
>>                 }
>>  #endif
>>
>> -               if (!opts->addr.port) {
>> -                       if (opts->ahmac) {
>> +               if (!addr_info->port) {
>> +                       if (!echo) {
>>                                 put_unaligned_be64(opts->ahmac, ptr);
>>                                 ptr += 2;
>>                         }
>>                 } else {
>> -                       u16 port = ntohs(opts->addr.port);
>> +                       u16 port = ntohs(addr_info->port);
>>
>> -                       if (opts->ahmac) {
>> +                       if (!echo) {
>>                                 u8 *bptr = (u8 *)ptr;
>>
>>                                 put_unaligned_be16(port, bptr);
>> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>                                 bptr += 8;
>>                                 put_unaligned_be16(TCPOPT_NOP << 8 |
>>                                                    TCPOPT_NOP, bptr);
>> -
>>                                 ptr += 3;
>>                         } else {
>>                                 put_unaligned_be32(port << 16 |
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index 107a5a2..a62d4a5 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>>
>>         lockdep_assert_held(&msk->pm.lock);
>>
>> -       if (add_addr) {
>> +       if (add_addr &
>> +           (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>                 pr_warn("addr_signal error, add_addr=%d", add_addr);
>>                 return -EINVAL;
>>         }
>> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>
>>  /* path manager helpers */
>>
>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
>> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
>> +                             struct mptcp_addr_info *daddr, u8 *add_addr)
>>  {
>> -       u8 add_addr;
>> -       int ret = false;
>> -
>>         spin_lock_bh(&msk->pm.lock);
>>
>> -       /* double check after the lock is acquired */
>> -       if (!mptcp_pm_should_add_signal(msk))
>> -               goto out_unlock;
>> -
>> -       *echo = mptcp_pm_should_add_signal_echo(msk);
>> -       *port = mptcp_pm_should_add_signal_port(msk);
>> -
>> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>> -               goto out_unlock;
>> -
>>         *saddr = msk->pm.local;
>> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
>> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
>> -       ret = true;
>> +       *daddr = msk->pm.remote;
>> +       *add_addr = msk->pm.addr_signal;
>>
>> -out_unlock:
>>         spin_unlock_bh(&msk->pm.lock);
>> -       return ret;
>> +
>> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>  }
>>
>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index a0b0ec0..90fb532 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -22,10 +22,11 @@
>>  #define OPTION_MPTCP_MPJ_SYNACK        BIT(4)
>>  #define OPTION_MPTCP_MPJ_ACK   BIT(5)
>>  #define OPTION_MPTCP_ADD_ADDR  BIT(6)
>> -#define OPTION_MPTCP_RM_ADDR   BIT(7)
>> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
>> -#define OPTION_MPTCP_PRIO      BIT(9)
>> -#define OPTION_MPTCP_RST       BIT(10)
>> +#define OPTION_MPTCP_ADD_ECHO  BIT(7)
>> +#define OPTION_MPTCP_RM_ADDR   BIT(8)
>> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
>> +#define OPTION_MPTCP_PRIO      BIT(10)
>> +#define OPTION_MPTCP_RST       BIT(11)
>>
>>  /* MPTCP option subtypes */
>>  #define MPTCPOPT_MP_CAPABLE    0
>> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>>  }
>>
>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
>> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
>> +                             struct mptcp_addr_info *daddr, u8 *add_addr);
>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>                              struct mptcp_rm_list *rm_list);
>>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>> --
>> 1.8.3.1
>>
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-21  3:51     ` Yonglong Li
@ 2021-06-21  6:42       ` Geliang Tang
  2021-06-21  7:15         ` Yonglong Li
  0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21  6:42 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng

Hi Yonglong,

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道:
>
>
>
> On 2021/6/18 19:20, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Thanks for v4!
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
> >>
> >> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >> ADD_ADDR/echo-ADD_ADDR option
> >>
> >> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>
> >> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >> ---
> >>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
> >>  net/mptcp/pm.c       |  30 ++++---------
> >>  net/mptcp/protocol.h |  13 +++---
> >>  3 files changed, 92 insertions(+), 75 deletions(-)
> >>
> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >> index 1aec016..43e3241 100644
> >> --- a/net/mptcp/options.c
> >> +++ b/net/mptcp/options.c
> >> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>         bool drop_other_suboptions = false;
> >>         unsigned int opt_size = *size;
> >> -       bool echo;
> >> -       bool port;
> >> +       struct mptcp_addr_info remote;
> >> +       struct mptcp_addr_info local;
> >> +       u8 add_addr, flags = 0xff;
> >>         int len;
> >>
> >> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >> -            mptcp_pm_should_add_signal_port(msk) ||
> >> -            mptcp_pm_should_add_signal_echo(msk)) &&
> >> -           skb && skb_is_tcp_pure_ack(skb)) {
> >> -               pr_debug("drop other suboptions");
> >> -               opts->suboptions = 0;
> >> -               opts->ext_copy.use_ack = 0;
> >> -               opts->ext_copy.use_map = 0;
> >> -               remaining += opt_size;
> >> -               drop_other_suboptions = true;
> >> -       }
> >> -
> >> -       if (!mptcp_pm_should_add_signal(msk) ||
> >> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >> -               return false;
> >> -
> >> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >> -       if (remaining < len)
> >> +       if (!mptcp_pm_should_add_signal(msk))
> >>                 return false;
> >>
> >> -       *size = len;
> >> -       if (drop_other_suboptions)
> >> -               *size -= opt_size;
> >> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >> -       if (!echo) {
> >> +       *size = 0;
> >> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> >> +               if (skb && skb_is_tcp_pure_ack(skb)) {
> >
> > '''
> >> +                       pr_debug("drop other suboptions");
> >> +                       opts->suboptions = 0;
> >> +                       opts->ext_copy.use_ack = 0;
> >> +                       opts->ext_copy.use_map = 0;
> >> +                       remaining += opt_size;
> >> +                       drop_other_suboptions = true;
> > '''
> >
> >> +               }
> >> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >> +               if (remaining < len)
> >> +                       return false;
> >> +               remaining -= len;
> >> +               *size += len;
> >> +               opts->remote = remote;
> >> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
> >> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >> +               if ((local.family == AF_INET6 || local.port) && skb &&
> >> +                   skb_is_tcp_pure_ack(skb)) {
> >
> > '''
> >> +                       pr_debug("drop other suboptions");
> >> +                       opts->suboptions = 0;
> >> +                       opts->ext_copy.use_ack = 0;
> >> +                       opts->ext_copy.use_map = 0;
> >> +                       remaining += opt_size;
> >> +                       drop_other_suboptions = true;
> > '''
> >
> > I think this "drop other suboptions" trunk here is still duplicated. Can
> > we just use one "drop other suboptions" trunk only?
> >
> > Thanks.
> > -Geliang
> >
> Hi Geliang, Thanks for you replay.
>
> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> IP version."
> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> to decide "drop other suboptions" in two trunk.

Could we change it like this:

'''
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index e77b5d532fb8..8b4cb0581a49 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -673,15 +673,20 @@ static bool
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *

        *size = 0;
        mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
+
+       if ((mptcp_pm_should_add_signal_echo(msk) ||
+           (mptcp_pm_should_add_signal_addr(msk) &&
+           (local.family == AF_INET6 || local.port))) &&
+           skb && skb_is_tcp_pure_ack(skb)) {
+               pr_debug("drop other suboptions");
+               opts->suboptions = 0;
+               opts->ext_copy.use_ack = 0;
+               opts->ext_copy.use_map = 0;
+               remaining += opt_size;
+               drop_other_suboptions = true;
+       }
+
        if (mptcp_pm_should_add_signal_echo(msk)) {
-               if (skb && skb_is_tcp_pure_ack(skb)) {
-                       pr_debug("drop other suboptions");
-                       opts->suboptions = 0;
-                       opts->ext_copy.use_ack = 0;
-                       opts->ext_copy.use_map = 0;
-                       remaining += opt_size;
-                       drop_other_suboptions = true;
-               }
                len = mptcp_add_addr_len(remote.family, true, !!remote.port);
                if (remaining < len)
                        return false;
@@ -693,15 +698,6 @@ static bool
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
                pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
                         opts->remote.id, ntohs(opts->remote.port), add_addr);
        } else if (mptcp_pm_should_add_signal_addr(msk)) {
-               if ((local.family == AF_INET6 || local.port) && skb &&
-                   skb_is_tcp_pure_ack(skb)) {
-                       pr_debug("drop other suboptions");
-                       opts->suboptions = 0;
-                       opts->ext_copy.use_ack = 0;
-                       opts->ext_copy.use_map = 0;
-                       remaining += opt_size;
-                       drop_other_suboptions = true;
-               }
                len = mptcp_add_addr_len(local.family, false, !!local.port);
                if (remaining < len)
                        return false;
'''
WDYT?

>
> >
> >
> >> +               }
> >> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
> >> +               if (remaining < len)
> >> +                       return false;

And here, I think "remaining -= len;" is missing.

Thanks,
-Geliang


> >> +               *size += len;
> >> +               opts->addr = local;
> >>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>                                                      msk->remote_key,
> >>                                                      &opts->addr);
> >> +               opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >> +               pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> >> +                        opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> >>         }
> >> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >> +
> >> +       if (drop_other_suboptions)
> >> +               *size -= opt_size;
> >> +       spin_lock_bh(&msk->pm.lock);
> >> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >> +       spin_unlock_bh(&msk->pm.lock);
> >>
> >>         return true;
> >>  }
> >> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>         }
> >>
> >>  mp_capable_done:
> >> -       if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >> -               u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >> -               u8 echo = MPTCP_ADDR_ECHO;
> >> +       if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> >> +               struct mptcp_addr_info *addr_info;
> >> +               u8 len = 0;
> >> +               u8 echo = 0;
> >> +
> >> +               if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >> +                       len += sizeof(opts->ahmac);
> >> +                       addr_info = &opts->addr;
> >> +               } else {
> >> +                       echo = MPTCP_ADDR_ECHO;
> >> +                       addr_info = &opts->remote;
> >> +               }
> >>
> >>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >> -               if (opts->addr.family == AF_INET6)
> >> -                       len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> +               if (addr_info->family == AF_INET6)
> >> +                       len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> +               else
> >>  #endif
> >> +                       len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>
> >> -               if (opts->addr.port)
> >> +               if (addr_info->port)
> >>                         len += TCPOLEN_MPTCP_PORT_LEN;
> >>
> >> -               if (opts->ahmac) {
> >> -                       len += sizeof(opts->ahmac);
> >> -                       echo = 0;
> >> -               }
> >> -
> >>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> >> -                                     len, echo, opts->addr.id);
> >> -               if (opts->addr.family == AF_INET) {
> >> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> >> +                                     len, echo, addr_info->id);
> >> +               if (addr_info->family == AF_INET) {
> >> +                       memcpy((u8 *)ptr, (u8 *)&addr_info->addr.s_addr, 4);
> >>                         ptr += 1;
> >>                 }
> >>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >> -               else if (opts->addr.family == AF_INET6) {
> >> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> >> +               else if (addr_info->family == AF_INET6) {
> >> +                       memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> >>                         ptr += 4;
> >>                 }
> >>  #endif
> >>
> >> -               if (!opts->addr.port) {
> >> -                       if (opts->ahmac) {
> >> +               if (!addr_info->port) {
> >> +                       if (!echo) {
> >>                                 put_unaligned_be64(opts->ahmac, ptr);
> >>                                 ptr += 2;
> >>                         }
> >>                 } else {
> >> -                       u16 port = ntohs(opts->addr.port);
> >> +                       u16 port = ntohs(addr_info->port);
> >>
> >> -                       if (opts->ahmac) {
> >> +                       if (!echo) {
> >>                                 u8 *bptr = (u8 *)ptr;
> >>
> >>                                 put_unaligned_be16(port, bptr);
> >> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>                                 bptr += 8;
> >>                                 put_unaligned_be16(TCPOPT_NOP << 8 |
> >>                                                    TCPOPT_NOP, bptr);
> >> -
> >>                                 ptr += 3;
> >>                         } else {
> >>                                 put_unaligned_be32(port << 16 |
> >> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >> index 107a5a2..a62d4a5 100644
> >> --- a/net/mptcp/pm.c
> >> +++ b/net/mptcp/pm.c
> >> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> >>
> >>         lockdep_assert_held(&msk->pm.lock);
> >>
> >> -       if (add_addr) {
> >> +       if (add_addr &
> >> +           (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> >>                 pr_warn("addr_signal error, add_addr=%d", add_addr);
> >>                 return -EINVAL;
> >>         }
> >> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>
> >>  /* path manager helpers */
> >>
> >> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> >> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> >> +                             struct mptcp_addr_info *daddr, u8 *add_addr)
> >>  {
> >> -       u8 add_addr;
> >> -       int ret = false;
> >> -
> >>         spin_lock_bh(&msk->pm.lock);
> >>
> >> -       /* double check after the lock is acquired */
> >> -       if (!mptcp_pm_should_add_signal(msk))
> >> -               goto out_unlock;
> >> -
> >> -       *echo = mptcp_pm_should_add_signal_echo(msk);
> >> -       *port = mptcp_pm_should_add_signal_port(msk);
> >> -
> >> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> >> -               goto out_unlock;
> >> -
> >>         *saddr = msk->pm.local;
> >> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> >> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >> -       ret = true;
> >> +       *daddr = msk->pm.remote;
> >> +       *add_addr = msk->pm.addr_signal;
> >>
> >> -out_unlock:
> >>         spin_unlock_bh(&msk->pm.lock);
> >> -       return ret;
> >> +
> >> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>  }
> >>
> >>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >> index a0b0ec0..90fb532 100644
> >> --- a/net/mptcp/protocol.h
> >> +++ b/net/mptcp/protocol.h
> >> @@ -22,10 +22,11 @@
> >>  #define OPTION_MPTCP_MPJ_SYNACK        BIT(4)
> >>  #define OPTION_MPTCP_MPJ_ACK   BIT(5)
> >>  #define OPTION_MPTCP_ADD_ADDR  BIT(6)
> >> -#define OPTION_MPTCP_RM_ADDR   BIT(7)
> >> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> >> -#define OPTION_MPTCP_PRIO      BIT(9)
> >> -#define OPTION_MPTCP_RST       BIT(10)
> >> +#define OPTION_MPTCP_ADD_ECHO  BIT(7)
> >> +#define OPTION_MPTCP_RM_ADDR   BIT(8)
> >> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> >> +#define OPTION_MPTCP_PRIO      BIT(10)
> >> +#define OPTION_MPTCP_RST       BIT(11)
> >>
> >>  /* MPTCP option subtypes */
> >>  #define MPTCPOPT_MP_CAPABLE    0
> >> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >>  }
> >>
> >> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> >> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> >> +                             struct mptcp_addr_info *daddr, u8 *add_addr);
> >>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>                              struct mptcp_rm_list *rm_list);
> >>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >> --
> >> 1.8.3.1
> >>
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-21  6:42       ` Geliang Tang
@ 2021-06-21  7:15         ` Yonglong Li
  2021-06-21  7:39           ` Geliang Tang
  0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21  7:15 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng



On 2021/6/21 14:42, Geliang Tang wrote:
> Hi Yonglong,
> 
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道:
>>
>>
>> On 2021/6/18 19:20, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Thanks for v4!
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>
>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>
>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>> ---
>>>>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
>>>>  net/mptcp/pm.c       |  30 ++++---------
>>>>  net/mptcp/protocol.h |  13 +++---
>>>>  3 files changed, 92 insertions(+), 75 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index 1aec016..43e3241 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>         bool drop_other_suboptions = false;
>>>>         unsigned int opt_size = *size;
>>>> -       bool echo;
>>>> -       bool port;
>>>> +       struct mptcp_addr_info remote;
>>>> +       struct mptcp_addr_info local;
>>>> +       u8 add_addr, flags = 0xff;
>>>>         int len;
>>>>
>>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>> -            mptcp_pm_should_add_signal_port(msk) ||
>>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
>>>> -           skb && skb_is_tcp_pure_ack(skb)) {
>>>> -               pr_debug("drop other suboptions");
>>>> -               opts->suboptions = 0;
>>>> -               opts->ext_copy.use_ack = 0;
>>>> -               opts->ext_copy.use_map = 0;
>>>> -               remaining += opt_size;
>>>> -               drop_other_suboptions = true;
>>>> -       }
>>>> -
>>>> -       if (!mptcp_pm_should_add_signal(msk) ||
>>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>> -               return false;
>>>> -
>>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>> -       if (remaining < len)
>>>> +       if (!mptcp_pm_should_add_signal(msk))
>>>>                 return false;
>>>>
>>>> -       *size = len;
>>>> -       if (drop_other_suboptions)
>>>> -               *size -= opt_size;
>>>> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>> -       if (!echo) {
>>>> +       *size = 0;
>>>> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
>>>> +               if (skb && skb_is_tcp_pure_ack(skb)) {
>>> '''
>>>> +                       pr_debug("drop other suboptions");
>>>> +                       opts->suboptions = 0;
>>>> +                       opts->ext_copy.use_ack = 0;
>>>> +                       opts->ext_copy.use_map = 0;
>>>> +                       remaining += opt_size;
>>>> +                       drop_other_suboptions = true;
>>> '''
>>>
>>>> +               }
>>>> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>>> +               if (remaining < len)
>>>> +                       return false;
>>>> +               remaining -= len;
>>>> +               *size += len;
>>>> +               opts->remote = remote;
>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>>>> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>>> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
>>>> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>>> +               if ((local.family == AF_INET6 || local.port) && skb &&
>>>> +                   skb_is_tcp_pure_ack(skb)) {
>>> '''
>>>> +                       pr_debug("drop other suboptions");
>>>> +                       opts->suboptions = 0;
>>>> +                       opts->ext_copy.use_ack = 0;
>>>> +                       opts->ext_copy.use_map = 0;
>>>> +                       remaining += opt_size;
>>>> +                       drop_other_suboptions = true;
>>> '''
>>>
>>> I think this "drop other suboptions" trunk here is still duplicated. Can
>>> we just use one "drop other suboptions" trunk only?
>>>
>>> Thanks.
>>> -Geliang
>>>
>> Hi Geliang, Thanks for you replay.
>>
>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
>> IP version."
>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
>> to decide "drop other suboptions" in two trunk.
> Could we change it like this:
> 
> '''
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index e77b5d532fb8..8b4cb0581a49 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -673,15 +673,20 @@ static bool
> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> 
>         *size = 0;
>         mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> +
> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> +           (mptcp_pm_should_add_signal_addr(msk) &&
> +           (local.family == AF_INET6 || local.port))) &&
> +           skb && skb_is_tcp_pure_ack(skb)) {
> +               pr_debug("drop other suboptions");
> +               opts->suboptions = 0;
> +               opts->ext_copy.use_ack = 0;
> +               opts->ext_copy.use_map = 0;
> +               remaining += opt_size;
> +               drop_other_suboptions = true;
> +       }
> +
>         if (mptcp_pm_should_add_signal_echo(msk)) {
> -               if (skb && skb_is_tcp_pure_ack(skb)) {
> -                       pr_debug("drop other suboptions");
> -                       opts->suboptions = 0;
> -                       opts->ext_copy.use_ack = 0;
> -                       opts->ext_copy.use_map = 0;
> -                       remaining += opt_size;
> -                       drop_other_suboptions = true;
> -               }
>                 len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>                 if (remaining < len)
>                         return false;
> @@ -693,15 +698,6 @@ static bool
> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>                 pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>                          opts->remote.id, ntohs(opts->remote.port), add_addr);
>         } else if (mptcp_pm_should_add_signal_addr(msk)) {
> -               if ((local.family == AF_INET6 || local.port) && skb &&
> -                   skb_is_tcp_pure_ack(skb)) {
> -                       pr_debug("drop other suboptions");
> -                       opts->suboptions = 0;
> -                       opts->ext_copy.use_ack = 0;
> -                       opts->ext_copy.use_map = 0;
> -                       remaining += opt_size;
> -                       drop_other_suboptions = true;
> -               }
>                 len = mptcp_add_addr_len(local.family, false, !!local.port);
>                 if (remaining < len)
>                         return false;
> '''
> WDYT?
Thanks for your advice.

Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
change like this(still I think it not clear than before):

        mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
+       if ((mptcp_pm_should_add_signal_echo(msk) ||
+           (!mptcp_pm_should_add_signal_echo(msk) &&
+           mptcp_pm_should_add_signal_addr(msk) &&
+           (local.family == AF_INET6 || local.port))) &&
+           skb && skb_is_tcp_pure_ack(skb)) {
+               pr_debug("drop other suboptions");
+               opts->suboptions = 0;
+               opts->ext_copy.use_ack = 0;
+               opts->ext_copy.use_map = 0;
+               remaining += opt_size;
+               drop_other_suboptions = true;
+       }
+
        if (mptcp_pm_should_add_signal_echo(msk)) {
-               if (skb && skb_is_tcp_pure_ack(skb)) {


>
>>>
>>>> +               }
>>>> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
>>>> +               if (remaining < len)
>>>> +                       return false;
> And here, I think "remaining -= len;" is missing.
> 
> Thanks,
> -Geliang
> 
"remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.

I will send v5 as your advice.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-21  7:15         ` Yonglong Li
@ 2021-06-21  7:39           ` Geliang Tang
  2021-06-21  7:49             ` Yonglong Li
  0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21  7:39 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道:
>
>
>
> On 2021/6/21 14:42, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道:
> >>
> >>
> >> On 2021/6/18 19:20, Geliang Tang wrote:
> >>> Hi Yonglong,
> >>>
> >>> Thanks for v4!
> >>>
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
> >>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>
> >>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>
> >>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>> ---
> >>>>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
> >>>>  net/mptcp/pm.c       |  30 ++++---------
> >>>>  net/mptcp/protocol.h |  13 +++---
> >>>>  3 files changed, 92 insertions(+), 75 deletions(-)
> >>>>
> >>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>> index 1aec016..43e3241 100644
> >>>> --- a/net/mptcp/options.c
> >>>> +++ b/net/mptcp/options.c
> >>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>         bool drop_other_suboptions = false;
> >>>>         unsigned int opt_size = *size;
> >>>> -       bool echo;
> >>>> -       bool port;
> >>>> +       struct mptcp_addr_info remote;
> >>>> +       struct mptcp_addr_info local;
> >>>> +       u8 add_addr, flags = 0xff;
> >>>>         int len;
> >>>>
> >>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>> -            mptcp_pm_should_add_signal_port(msk) ||
> >>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
> >>>> -           skb && skb_is_tcp_pure_ack(skb)) {
> >>>> -               pr_debug("drop other suboptions");
> >>>> -               opts->suboptions = 0;
> >>>> -               opts->ext_copy.use_ack = 0;
> >>>> -               opts->ext_copy.use_map = 0;
> >>>> -               remaining += opt_size;
> >>>> -               drop_other_suboptions = true;
> >>>> -       }
> >>>> -
> >>>> -       if (!mptcp_pm_should_add_signal(msk) ||
> >>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>> -               return false;
> >>>> -
> >>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>> -       if (remaining < len)
> >>>> +       if (!mptcp_pm_should_add_signal(msk))
> >>>>                 return false;
> >>>>
> >>>> -       *size = len;
> >>>> -       if (drop_other_suboptions)
> >>>> -               *size -= opt_size;
> >>>> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>> -       if (!echo) {
> >>>> +       *size = 0;
> >>>> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>> +               if (skb && skb_is_tcp_pure_ack(skb)) {
> >>> '''
> >>>> +                       pr_debug("drop other suboptions");
> >>>> +                       opts->suboptions = 0;
> >>>> +                       opts->ext_copy.use_ack = 0;
> >>>> +                       opts->ext_copy.use_map = 0;
> >>>> +                       remaining += opt_size;
> >>>> +                       drop_other_suboptions = true;
> >>> '''
> >>>
> >>>> +               }
> >>>> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>>> +               if (remaining < len)
> >>>> +                       return false;
> >>>> +               remaining -= len;
> >>>> +               *size += len;
> >>>> +               opts->remote = remote;
> >>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >>>> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>>> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>>> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>> +               if ((local.family == AF_INET6 || local.port) && skb &&
> >>>> +                   skb_is_tcp_pure_ack(skb)) {
> >>> '''
> >>>> +                       pr_debug("drop other suboptions");
> >>>> +                       opts->suboptions = 0;
> >>>> +                       opts->ext_copy.use_ack = 0;
> >>>> +                       opts->ext_copy.use_map = 0;
> >>>> +                       remaining += opt_size;
> >>>> +                       drop_other_suboptions = true;
> >>> '''
> >>>
> >>> I think this "drop other suboptions" trunk here is still duplicated. Can
> >>> we just use one "drop other suboptions" trunk only?
> >>>
> >>> Thanks.
> >>> -Geliang
> >>>
> >> Hi Geliang, Thanks for you replay.
> >>
> >> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> >> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> >> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> >> IP version."
> >> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> >> to decide "drop other suboptions" in two trunk.
> > Could we change it like this:
> >
> > '''
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index e77b5d532fb8..8b4cb0581a49 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -673,15 +673,20 @@ static bool
> > mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >
> >         *size = 0;
> >         mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> > +
> > +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> > +           (mptcp_pm_should_add_signal_addr(msk) &&
> > +           (local.family == AF_INET6 || local.port))) &&
> > +           skb && skb_is_tcp_pure_ack(skb)) {
> > +               pr_debug("drop other suboptions");
> > +               opts->suboptions = 0;
> > +               opts->ext_copy.use_ack = 0;
> > +               opts->ext_copy.use_map = 0;
> > +               remaining += opt_size;
> > +               drop_other_suboptions = true;
> > +       }
> > +
> >         if (mptcp_pm_should_add_signal_echo(msk)) {
> > -               if (skb && skb_is_tcp_pure_ack(skb)) {
> > -                       pr_debug("drop other suboptions");
> > -                       opts->suboptions = 0;
> > -                       opts->ext_copy.use_ack = 0;
> > -                       opts->ext_copy.use_map = 0;
> > -                       remaining += opt_size;
> > -                       drop_other_suboptions = true;
> > -               }
> >                 len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >                 if (remaining < len)
> >                         return false;
> > @@ -693,15 +698,6 @@ static bool
> > mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >                 pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >                          opts->remote.id, ntohs(opts->remote.port), add_addr);
> >         } else if (mptcp_pm_should_add_signal_addr(msk)) {
> > -               if ((local.family == AF_INET6 || local.port) && skb &&
> > -                   skb_is_tcp_pure_ack(skb)) {
> > -                       pr_debug("drop other suboptions");
> > -                       opts->suboptions = 0;
> > -                       opts->ext_copy.use_ack = 0;
> > -                       opts->ext_copy.use_map = 0;
> > -                       remaining += opt_size;
> > -                       drop_other_suboptions = true;
> > -               }
> >                 len = mptcp_add_addr_len(local.family, false, !!local.port);
> >                 if (remaining < len)
> >                         return false;
> > '''
> > WDYT?
> Thanks for your advice.
>
> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
> change like this(still I think it not clear than before):
>
>         mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> +           (!mptcp_pm_should_add_signal_echo(msk) &&
> +           mptcp_pm_should_add_signal_addr(msk) &&
> +           (local.family == AF_INET6 || local.port))) &&
> +           skb && skb_is_tcp_pure_ack(skb)) {
> +               pr_debug("drop other suboptions");
> +               opts->suboptions = 0;
> +               opts->ext_copy.use_ack = 0;
> +               opts->ext_copy.use_map = 0;
> +               remaining += opt_size;
> +               drop_other_suboptions = true;
> +       }
> +
>         if (mptcp_pm_should_add_signal_echo(msk)) {
> -               if (skb && skb_is_tcp_pure_ack(skb)) {
>
>
> >
> >>>
> >>>> +               }
> >>>> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>>> +               if (remaining < len)
> >>>> +                       return false;
> > And here, I think "remaining -= len;" is missing.
> >
> > Thanks,
> > -Geliang
> >
> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.

I think we should keep this 'remaining -= len;', remaining can be used
in tcp_established_options.

>
> I will send v5 as your advice.
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-18  8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
  2021-06-18 11:20   ` Geliang Tang
@ 2021-06-21  7:42   ` Geliang Tang
  2021-06-21  7:51     ` Yonglong Li
  2021-06-21  8:29   ` Geliang Tang
  2 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21  7:42 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
>  net/mptcp/pm.c       |  30 ++++---------
>  net/mptcp/protocol.h |  13 +++---
>  3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>         bool drop_other_suboptions = false;
>         unsigned int opt_size = *size;
> -       bool echo;
> -       bool port;
> +       struct mptcp_addr_info remote;
> +       struct mptcp_addr_info local;
> +       u8 add_addr, flags = 0xff;
>         int len;
>
> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> -            mptcp_pm_should_add_signal_port(msk) ||
> -            mptcp_pm_should_add_signal_echo(msk)) &&
> -           skb && skb_is_tcp_pure_ack(skb)) {
> -               pr_debug("drop other suboptions");
> -               opts->suboptions = 0;
> -               opts->ext_copy.use_ack = 0;
> -               opts->ext_copy.use_map = 0;
> -               remaining += opt_size;
> -               drop_other_suboptions = true;
> -       }
> -
> -       if (!mptcp_pm_should_add_signal(msk) ||
> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> -               return false;
> -
> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> -       if (remaining < len)
> +       if (!mptcp_pm_should_add_signal(msk))
>                 return false;
>
> -       *size = len;
> -       if (drop_other_suboptions)
> -               *size -= opt_size;
> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> -       if (!echo) {
> +       *size = 0;
> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> +               if (skb && skb_is_tcp_pure_ack(skb)) {
> +                       pr_debug("drop other suboptions");
> +                       opts->suboptions = 0;
> +                       opts->ext_copy.use_ack = 0;
> +                       opts->ext_copy.use_map = 0;
> +                       remaining += opt_size;
> +                       drop_other_suboptions = true;
> +               }
> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> +               if (remaining < len)
> +                       return false;
> +               remaining -= len;
> +               *size += len;
> +               opts->remote = remote;
> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {
> +               if ((local.family == AF_INET6 || local.port) && skb &&
> +                   skb_is_tcp_pure_ack(skb)) {
> +                       pr_debug("drop other suboptions");
> +                       opts->suboptions = 0;
> +                       opts->ext_copy.use_ack = 0;
> +                       opts->ext_copy.use_map = 0;
> +                       remaining += opt_size;
> +                       drop_other_suboptions = true;
> +               }
> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
> +               if (remaining < len)
> +                       return false;
> +               *size += len;
> +               opts->addr = local;

Could we rename this struct member addr in struct mptcp_out_options to
local?

>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>                                                      msk->remote_key,
>                                                      &opts->addr);
> +               opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> +               pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> +                        opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);

Could we merge these two debug logs into one and move it at the the end
of this function, before 'return true'?

-Geliang

>         }
> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> +       if (drop_other_suboptions)
> +               *size -= opt_size;
> +       spin_lock_bh(&msk->pm.lock);
> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> +       spin_unlock_bh(&msk->pm.lock);
>
>         return true;
>  }
> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>         }
>
>  mp_capable_done:
> -       if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> -               u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> -               u8 echo = MPTCP_ADDR_ECHO;
> +       if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> +               struct mptcp_addr_info *addr_info;
> +               u8 len = 0;
> +               u8 echo = 0;
> +
> +               if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> +                       len += sizeof(opts->ahmac);
> +                       addr_info = &opts->addr;
> +               } else {
> +                       echo = MPTCP_ADDR_ECHO;
> +                       addr_info = &opts->remote;
> +               }
>
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -               if (opts->addr.family == AF_INET6)
> -                       len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +               if (addr_info->family == AF_INET6)
> +                       len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +               else
>  #endif
> +                       len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> -               if (opts->addr.port)
> +               if (addr_info->port)
>                         len += TCPOLEN_MPTCP_PORT_LEN;
>
> -               if (opts->ahmac) {
> -                       len += sizeof(opts->ahmac);
> -                       echo = 0;
> -               }
> -
>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> -                                     len, echo, opts->addr.id);
> -               if (opts->addr.family == AF_INET) {
> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> +                                     len, echo, addr_info->id);
> +               if (addr_info->family == AF_INET) {
> +                       memcpy((u8 *)ptr, (u8 *)&addr_info->addr.s_addr, 4);
>                         ptr += 1;
>                 }
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -               else if (opts->addr.family == AF_INET6) {
> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> +               else if (addr_info->family == AF_INET6) {
> +                       memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
>                         ptr += 4;
>                 }
>  #endif
>
> -               if (!opts->addr.port) {
> -                       if (opts->ahmac) {
> +               if (!addr_info->port) {
> +                       if (!echo) {
>                                 put_unaligned_be64(opts->ahmac, ptr);
>                                 ptr += 2;
>                         }
>                 } else {
> -                       u16 port = ntohs(opts->addr.port);
> +                       u16 port = ntohs(addr_info->port);
>
> -                       if (opts->ahmac) {
> +                       if (!echo) {
>                                 u8 *bptr = (u8 *)ptr;
>
>                                 put_unaligned_be16(port, bptr);
> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>                                 bptr += 8;
>                                 put_unaligned_be16(TCPOPT_NOP << 8 |
>                                                    TCPOPT_NOP, bptr);
> -
>                                 ptr += 3;
>                         } else {
>                                 put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..a62d4a5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
>         lockdep_assert_held(&msk->pm.lock);
>
> -       if (add_addr) {
> +       if (add_addr &
> +           (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>                 pr_warn("addr_signal error, add_addr=%d", add_addr);
>                 return -EINVAL;
>         }
> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
>  /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> +                             struct mptcp_addr_info *daddr, u8 *add_addr)
>  {
> -       u8 add_addr;
> -       int ret = false;
> -
>         spin_lock_bh(&msk->pm.lock);
>
> -       /* double check after the lock is acquired */
> -       if (!mptcp_pm_should_add_signal(msk))
> -               goto out_unlock;
> -
> -       *echo = mptcp_pm_should_add_signal_echo(msk);
> -       *port = mptcp_pm_should_add_signal_port(msk);
> -
> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> -               goto out_unlock;
> -
>         *saddr = msk->pm.local;
> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
> -       ret = true;
> +       *daddr = msk->pm.remote;
> +       *add_addr = msk->pm.addr_signal;
>
> -out_unlock:
>         spin_unlock_bh(&msk->pm.lock);
> -       return ret;
> +
> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>  }
>
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0b0ec0..90fb532 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -22,10 +22,11 @@
>  #define OPTION_MPTCP_MPJ_SYNACK        BIT(4)
>  #define OPTION_MPTCP_MPJ_ACK   BIT(5)
>  #define OPTION_MPTCP_ADD_ADDR  BIT(6)
> -#define OPTION_MPTCP_RM_ADDR   BIT(7)
> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> -#define OPTION_MPTCP_PRIO      BIT(9)
> -#define OPTION_MPTCP_RST       BIT(10)
> +#define OPTION_MPTCP_ADD_ECHO  BIT(7)
> +#define OPTION_MPTCP_RM_ADDR   BIT(8)
> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> +#define OPTION_MPTCP_PRIO      BIT(10)
> +#define OPTION_MPTCP_RST       BIT(11)
>
>  /* MPTCP option subtypes */
>  #define MPTCPOPT_MP_CAPABLE    0
> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>  }
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> +                             struct mptcp_addr_info *daddr, u8 *add_addr);
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>                              struct mptcp_rm_list *rm_list);
>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> --
> 1.8.3.1
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-21  7:39           ` Geliang Tang
@ 2021-06-21  7:49             ` Yonglong Li
  2021-06-21  8:06               ` Geliang Tang
  0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21  7:49 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng



On 2021/6/21 15:39, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道:
>>
>>
>>
>> On 2021/6/21 14:42, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道:
>>>>
>>>>
>>>> On 2021/6/18 19:20, Geliang Tang wrote:
>>>>> Hi Yonglong,
>>>>>
>>>>> Thanks for v4!
>>>>>
>>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
>>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>>>
>>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>>>
>>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>>>> ---
>>>>>>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
>>>>>>  net/mptcp/pm.c       |  30 ++++---------
>>>>>>  net/mptcp/protocol.h |  13 +++---
>>>>>>  3 files changed, 92 insertions(+), 75 deletions(-)
>>>>>>
>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>> index 1aec016..43e3241 100644
>>>>>> --- a/net/mptcp/options.c
>>>>>> +++ b/net/mptcp/options.c
>>>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>>         bool drop_other_suboptions = false;
>>>>>>         unsigned int opt_size = *size;
>>>>>> -       bool echo;
>>>>>> -       bool port;
>>>>>> +       struct mptcp_addr_info remote;
>>>>>> +       struct mptcp_addr_info local;
>>>>>> +       u8 add_addr, flags = 0xff;
>>>>>>         int len;
>>>>>>
>>>>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>>>> -            mptcp_pm_should_add_signal_port(msk) ||
>>>>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
>>>>>> -           skb && skb_is_tcp_pure_ack(skb)) {
>>>>>> -               pr_debug("drop other suboptions");
>>>>>> -               opts->suboptions = 0;
>>>>>> -               opts->ext_copy.use_ack = 0;
>>>>>> -               opts->ext_copy.use_map = 0;
>>>>>> -               remaining += opt_size;
>>>>>> -               drop_other_suboptions = true;
>>>>>> -       }
>>>>>> -
>>>>>> -       if (!mptcp_pm_should_add_signal(msk) ||
>>>>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>>>> -               return false;
>>>>>> -
>>>>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>>>> -       if (remaining < len)
>>>>>> +       if (!mptcp_pm_should_add_signal(msk))
>>>>>>                 return false;
>>>>>>
>>>>>> -       *size = len;
>>>>>> -       if (drop_other_suboptions)
>>>>>> -               *size -= opt_size;
>>>>>> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>> -       if (!echo) {
>>>>>> +       *size = 0;
>>>>>> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>>>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
>>>>>> +               if (skb && skb_is_tcp_pure_ack(skb)) {
>>>>> '''
>>>>>> +                       pr_debug("drop other suboptions");
>>>>>> +                       opts->suboptions = 0;
>>>>>> +                       opts->ext_copy.use_ack = 0;
>>>>>> +                       opts->ext_copy.use_map = 0;
>>>>>> +                       remaining += opt_size;
>>>>>> +                       drop_other_suboptions = true;
>>>>> '''
>>>>>
>>>>>> +               }
>>>>>> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>>>>> +               if (remaining < len)
>>>>>> +                       return false;
>>>>>> +               remaining -= len;
>>>>>> +               *size += len;
>>>>>> +               opts->remote = remote;
>>>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>>>>>> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>>>>> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
>>>>>> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>>>>> +               if ((local.family == AF_INET6 || local.port) && skb &&
>>>>>> +                   skb_is_tcp_pure_ack(skb)) {
>>>>> '''
>>>>>> +                       pr_debug("drop other suboptions");
>>>>>> +                       opts->suboptions = 0;
>>>>>> +                       opts->ext_copy.use_ack = 0;
>>>>>> +                       opts->ext_copy.use_map = 0;
>>>>>> +                       remaining += opt_size;
>>>>>> +                       drop_other_suboptions = true;
>>>>> '''
>>>>>
>>>>> I think this "drop other suboptions" trunk here is still duplicated. Can
>>>>> we just use one "drop other suboptions" trunk only?
>>>>>
>>>>> Thanks.
>>>>> -Geliang
>>>>>
>>>> Hi Geliang, Thanks for you replay.
>>>>
>>>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
>>>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
>>>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
>>>> IP version."
>>>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
>>>> to decide "drop other suboptions" in two trunk.
>>> Could we change it like this:
>>>
>>> '''
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index e77b5d532fb8..8b4cb0581a49 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -673,15 +673,20 @@ static bool
>>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>
>>>         *size = 0;
>>>         mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>> +
>>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
>>> +           (mptcp_pm_should_add_signal_addr(msk) &&
>>> +           (local.family == AF_INET6 || local.port))) &&
>>> +           skb && skb_is_tcp_pure_ack(skb)) {
>>> +               pr_debug("drop other suboptions");
>>> +               opts->suboptions = 0;
>>> +               opts->ext_copy.use_ack = 0;
>>> +               opts->ext_copy.use_map = 0;
>>> +               remaining += opt_size;
>>> +               drop_other_suboptions = true;
>>> +       }
>>> +
>>>         if (mptcp_pm_should_add_signal_echo(msk)) {
>>> -               if (skb && skb_is_tcp_pure_ack(skb)) {
>>> -                       pr_debug("drop other suboptions");
>>> -                       opts->suboptions = 0;
>>> -                       opts->ext_copy.use_ack = 0;
>>> -                       opts->ext_copy.use_map = 0;
>>> -                       remaining += opt_size;
>>> -                       drop_other_suboptions = true;
>>> -               }
>>>                 len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>>                 if (remaining < len)
>>>                         return false;
>>> @@ -693,15 +698,6 @@ static bool
>>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>                 pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>>                          opts->remote.id, ntohs(opts->remote.port), add_addr);
>>>         } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>> -               if ((local.family == AF_INET6 || local.port) && skb &&
>>> -                   skb_is_tcp_pure_ack(skb)) {
>>> -                       pr_debug("drop other suboptions");
>>> -                       opts->suboptions = 0;
>>> -                       opts->ext_copy.use_ack = 0;
>>> -                       opts->ext_copy.use_map = 0;
>>> -                       remaining += opt_size;
>>> -                       drop_other_suboptions = true;
>>> -               }
>>>                 len = mptcp_add_addr_len(local.family, false, !!local.port);
>>>                 if (remaining < len)
>>>                         return false;
>>> '''
>>> WDYT?
>> Thanks for your advice.
>>
>> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
>> change like this(still I think it not clear than before):
>>
>>         mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
>> +           (!mptcp_pm_should_add_signal_echo(msk) &&
>> +           mptcp_pm_should_add_signal_addr(msk) &&
>> +           (local.family == AF_INET6 || local.port))) &&
>> +           skb && skb_is_tcp_pure_ack(skb)) {
>> +               pr_debug("drop other suboptions");
>> +               opts->suboptions = 0;
>> +               opts->ext_copy.use_ack = 0;
>> +               opts->ext_copy.use_map = 0;
>> +               remaining += opt_size;
>> +               drop_other_suboptions = true;
>> +       }
>> +
>>         if (mptcp_pm_should_add_signal_echo(msk)) {
>> -               if (skb && skb_is_tcp_pure_ack(skb)) {
>>
>>
>>>
>>>>>
>>>>>> +               }
>>>>>> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
>>>>>> +               if (remaining < len)
>>>>>> +                       return false;
>>> And here, I think "remaining -= len;" is missing.
>>>
>>> Thanks,
>>> -Geliang
>>>
>> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
> 
> I think we should keep this 'remaining -= len;', remaining can be used
> in tcp_established_options.
> 
Thanks for your review.
I think "remaining" will not use in tcp_established_options. "size" is used by tcp_established_options.

>>
>> I will send v5 as your advice.
>>
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-21  7:42   ` Geliang Tang
@ 2021-06-21  7:51     ` Yonglong Li
  0 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-21  7:51 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp, Mat Martineau



On 2021/6/21 15:42, Geliang Tang wrote:
>> +               }
>> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
>> +               if (remaining < len)
>> +                       return false;
>> +               *size += len;
>> +               opts->addr = local;
> Could we rename this struct member addr in struct mptcp_out_options to
> local?
> 
>>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>                                                      msk->remote_key,
>>                                                      &opts->addr);
>> +               opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> +               pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
>> +                        opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> Could we merge these two debug logs into one and move it at the the end
> of this function, before 'return true'?
> 
> -Geliang
> 
Thanks for your review.

I will change them in v5 as your advice.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-21  7:49             ` Yonglong Li
@ 2021-06-21  8:06               ` Geliang Tang
  0 siblings, 0 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-21  8:06 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:50写道:
>
>
>
> On 2021/6/21 15:39, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道:
> >>
> >>
> >>
> >> On 2021/6/21 14:42, Geliang Tang wrote:
> >>> Hi Yonglong,
> >>>
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道:
> >>>>
> >>>>
> >>>> On 2021/6/18 19:20, Geliang Tang wrote:
> >>>>> Hi Yonglong,
> >>>>>
> >>>>> Thanks for v4!
> >>>>>
> >>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
> >>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>>>
> >>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>>>
> >>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>>>> ---
> >>>>>>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
> >>>>>>  net/mptcp/pm.c       |  30 ++++---------
> >>>>>>  net/mptcp/protocol.h |  13 +++---
> >>>>>>  3 files changed, 92 insertions(+), 75 deletions(-)
> >>>>>>
> >>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>>> index 1aec016..43e3241 100644
> >>>>>> --- a/net/mptcp/options.c
> >>>>>> +++ b/net/mptcp/options.c
> >>>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>>>         bool drop_other_suboptions = false;
> >>>>>>         unsigned int opt_size = *size;
> >>>>>> -       bool echo;
> >>>>>> -       bool port;
> >>>>>> +       struct mptcp_addr_info remote;
> >>>>>> +       struct mptcp_addr_info local;
> >>>>>> +       u8 add_addr, flags = 0xff;
> >>>>>>         int len;
> >>>>>>
> >>>>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>>>> -            mptcp_pm_should_add_signal_port(msk) ||
> >>>>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
> >>>>>> -           skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>> -               pr_debug("drop other suboptions");
> >>>>>> -               opts->suboptions = 0;
> >>>>>> -               opts->ext_copy.use_ack = 0;
> >>>>>> -               opts->ext_copy.use_map = 0;
> >>>>>> -               remaining += opt_size;
> >>>>>> -               drop_other_suboptions = true;
> >>>>>> -       }
> >>>>>> -
> >>>>>> -       if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>>>> -               return false;
> >>>>>> -
> >>>>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>>>> -       if (remaining < len)
> >>>>>> +       if (!mptcp_pm_should_add_signal(msk))
> >>>>>>                 return false;
> >>>>>>
> >>>>>> -       *size = len;
> >>>>>> -       if (drop_other_suboptions)
> >>>>>> -               *size -= opt_size;
> >>>>>> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>>>> -       if (!echo) {
> >>>>>> +       *size = 0;
> >>>>>> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>>>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>>>> +               if (skb && skb_is_tcp_pure_ack(skb)) {
> >>>>> '''
> >>>>>> +                       pr_debug("drop other suboptions");
> >>>>>> +                       opts->suboptions = 0;
> >>>>>> +                       opts->ext_copy.use_ack = 0;
> >>>>>> +                       opts->ext_copy.use_map = 0;
> >>>>>> +                       remaining += opt_size;
> >>>>>> +                       drop_other_suboptions = true;
> >>>>> '''
> >>>>>
> >>>>>> +               }
> >>>>>> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>>>>> +               if (remaining < len)
> >>>>>> +                       return false;
> >>>>>> +               remaining -= len;
> >>>>>> +               *size += len;
> >>>>>> +               opts->remote = remote;
> >>>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>>> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >>>>>> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>>>>> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>>>>> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>>>> +               if ((local.family == AF_INET6 || local.port) && skb &&
> >>>>>> +                   skb_is_tcp_pure_ack(skb)) {
> >>>>> '''
> >>>>>> +                       pr_debug("drop other suboptions");
> >>>>>> +                       opts->suboptions = 0;
> >>>>>> +                       opts->ext_copy.use_ack = 0;
> >>>>>> +                       opts->ext_copy.use_map = 0;
> >>>>>> +                       remaining += opt_size;
> >>>>>> +                       drop_other_suboptions = true;
> >>>>> '''
> >>>>>
> >>>>> I think this "drop other suboptions" trunk here is still duplicated. Can
> >>>>> we just use one "drop other suboptions" trunk only?
> >>>>>
> >>>>> Thanks.
> >>>>> -Geliang
> >>>>>
> >>>> Hi Geliang, Thanks for you replay.
> >>>>
> >>>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> >>>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> >>>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> >>>> IP version."
> >>>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> >>>> to decide "drop other suboptions" in two trunk.
> >>> Could we change it like this:
> >>>
> >>> '''
> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>> index e77b5d532fb8..8b4cb0581a49 100644
> >>> --- a/net/mptcp/options.c
> >>> +++ b/net/mptcp/options.c
> >>> @@ -673,15 +673,20 @@ static bool
> >>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>
> >>>         *size = 0;
> >>>         mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>> +
> >>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>> +           (mptcp_pm_should_add_signal_addr(msk) &&
> >>> +           (local.family == AF_INET6 || local.port))) &&
> >>> +           skb && skb_is_tcp_pure_ack(skb)) {
> >>> +               pr_debug("drop other suboptions");
> >>> +               opts->suboptions = 0;
> >>> +               opts->ext_copy.use_ack = 0;
> >>> +               opts->ext_copy.use_map = 0;
> >>> +               remaining += opt_size;
> >>> +               drop_other_suboptions = true;
> >>> +       }
> >>> +
> >>>         if (mptcp_pm_should_add_signal_echo(msk)) {
> >>> -               if (skb && skb_is_tcp_pure_ack(skb)) {
> >>> -                       pr_debug("drop other suboptions");
> >>> -                       opts->suboptions = 0;
> >>> -                       opts->ext_copy.use_ack = 0;
> >>> -                       opts->ext_copy.use_map = 0;
> >>> -                       remaining += opt_size;
> >>> -                       drop_other_suboptions = true;
> >>> -               }
> >>>                 len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>>                 if (remaining < len)
> >>>                         return false;
> >>> @@ -693,15 +698,6 @@ static bool
> >>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>                 pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>>                          opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>>         } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>> -               if ((local.family == AF_INET6 || local.port) && skb &&
> >>> -                   skb_is_tcp_pure_ack(skb)) {
> >>> -                       pr_debug("drop other suboptions");
> >>> -                       opts->suboptions = 0;
> >>> -                       opts->ext_copy.use_ack = 0;
> >>> -                       opts->ext_copy.use_map = 0;
> >>> -                       remaining += opt_size;
> >>> -                       drop_other_suboptions = true;
> >>> -               }
> >>>                 len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>>                 if (remaining < len)
> >>>                         return false;
> >>> '''
> >>> WDYT?
> >> Thanks for your advice.
> >>
> >> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
> >> change like this(still I think it not clear than before):
> >>
> >>         mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> >> +           (!mptcp_pm_should_add_signal_echo(msk) &&
> >> +           mptcp_pm_should_add_signal_addr(msk) &&
> >> +           (local.family == AF_INET6 || local.port))) &&
> >> +           skb && skb_is_tcp_pure_ack(skb)) {
> >> +               pr_debug("drop other suboptions");
> >> +               opts->suboptions = 0;
> >> +               opts->ext_copy.use_ack = 0;
> >> +               opts->ext_copy.use_map = 0;
> >> +               remaining += opt_size;
> >> +               drop_other_suboptions = true;
> >> +       }
> >> +
> >>         if (mptcp_pm_should_add_signal_echo(msk)) {
> >> -               if (skb && skb_is_tcp_pure_ack(skb)) {
> >>
> >>
> >>>
> >>>>>
> >>>>>> +               }
> >>>>>> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>>>>> +               if (remaining < len)
> >>>>>> +                       return false;
> >>> And here, I think "remaining -= len;" is missing.
> >>>
> >>> Thanks,
> >>> -Geliang
> >>>
> >> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
> >
> > I think we should keep this 'remaining -= len;', remaining can be used
> > in tcp_established_options.
> >
> Thanks for your review.
> I think "remaining" will not use in tcp_established_options. "size" is used by tcp_established_options.

You're right, we should drop this 'remaining -= len;' in this function.

>
> >>
> >> I will send v5 as your advice.
> >>
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-18  8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
  2021-06-18 11:20   ` Geliang Tang
  2021-06-21  7:42   ` Geliang Tang
@ 2021-06-21  8:29   ` Geliang Tang
  2 siblings, 0 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-21  8:29 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道:
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
>  net/mptcp/options.c  | 124 +++++++++++++++++++++++++++++++--------------------
>  net/mptcp/pm.c       |  30 ++++---------
>  net/mptcp/protocol.h |  13 +++---
>  3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>         bool drop_other_suboptions = false;
>         unsigned int opt_size = *size;
> -       bool echo;
> -       bool port;
> +       struct mptcp_addr_info remote;
> +       struct mptcp_addr_info local;
> +       u8 add_addr, flags = 0xff;
>         int len;
>
> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> -            mptcp_pm_should_add_signal_port(msk) ||
> -            mptcp_pm_should_add_signal_echo(msk)) &&
> -           skb && skb_is_tcp_pure_ack(skb)) {
> -               pr_debug("drop other suboptions");
> -               opts->suboptions = 0;
> -               opts->ext_copy.use_ack = 0;
> -               opts->ext_copy.use_map = 0;
> -               remaining += opt_size;
> -               drop_other_suboptions = true;
> -       }
> -
> -       if (!mptcp_pm_should_add_signal(msk) ||
> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> -               return false;
> -
> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> -       if (remaining < len)
> +       if (!mptcp_pm_should_add_signal(msk))
>                 return false;
>
> -       *size = len;
> -       if (drop_other_suboptions)
> -               *size -= opt_size;
> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> -       if (!echo) {
> +       *size = 0;
> +       mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> +               if (skb && skb_is_tcp_pure_ack(skb)) {
> +                       pr_debug("drop other suboptions");
> +                       opts->suboptions = 0;
> +                       opts->ext_copy.use_ack = 0;
> +                       opts->ext_copy.use_map = 0;
> +                       remaining += opt_size;
> +                       drop_other_suboptions = true;
> +               }
> +               len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> +               if (remaining < len)
> +                       return false;
> +               remaining -= len;
> +               *size += len;

Could we drop the above '*size = 0', change this line to "*size = len;",
and move it out of the if... else... trunk, just like the original code:

       *size = len;
       if (drop_other_suboptions)
               *size -= opt_size;

> +               opts->remote = remote;
> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> +               opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> +               pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> +                        opts->remote.id, ntohs(opts->remote.port), add_addr);
> +       } else if (mptcp_pm_should_add_signal_addr(msk)) {

Since we called mptcp_pm_should_add_signal before, could we just use
'else' here?

-Geliang

> +               if ((local.family == AF_INET6 || local.port) && skb &&
> +                   skb_is_tcp_pure_ack(skb)) {
> +                       pr_debug("drop other suboptions");
> +                       opts->suboptions = 0;
> +                       opts->ext_copy.use_ack = 0;
> +                       opts->ext_copy.use_map = 0;
> +                       remaining += opt_size;
> +                       drop_other_suboptions = true;
> +               }
> +               len = mptcp_add_addr_len(local.family, false, !!local.port);
> +               if (remaining < len)
> +                       return false;
> +               *size += len;
> +               opts->addr = local;
>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>                                                      msk->remote_key,
>                                                      &opts->addr);
> +               opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> +               pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> +                        opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
>         }
> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> +       if (drop_other_suboptions)
> +               *size -= opt_size;
> +       spin_lock_bh(&msk->pm.lock);
> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> +       spin_unlock_bh(&msk->pm.lock);
>
>         return true;
>  }
> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>         }
>
>  mp_capable_done:
> -       if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> -               u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> -               u8 echo = MPTCP_ADDR_ECHO;
> +       if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> +               struct mptcp_addr_info *addr_info;
> +               u8 len = 0;
> +               u8 echo = 0;
> +
> +               if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> +                       len += sizeof(opts->ahmac);
> +                       addr_info = &opts->addr;
> +               } else {
> +                       echo = MPTCP_ADDR_ECHO;
> +                       addr_info = &opts->remote;
> +               }
>
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -               if (opts->addr.family == AF_INET6)
> -                       len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +               if (addr_info->family == AF_INET6)
> +                       len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +               else
>  #endif
> +                       len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> -               if (opts->addr.port)
> +               if (addr_info->port)
>                         len += TCPOLEN_MPTCP_PORT_LEN;
>
> -               if (opts->ahmac) {
> -                       len += sizeof(opts->ahmac);
> -                       echo = 0;
> -               }
> -
>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> -                                     len, echo, opts->addr.id);
> -               if (opts->addr.family == AF_INET) {
> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> +                                     len, echo, addr_info->id);
> +               if (addr_info->family == AF_INET) {
> +                       memcpy((u8 *)ptr, (u8 *)&addr_info->addr.s_addr, 4);
>                         ptr += 1;
>                 }
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -               else if (opts->addr.family == AF_INET6) {
> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> +               else if (addr_info->family == AF_INET6) {
> +                       memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
>                         ptr += 4;
>                 }
>  #endif
>
> -               if (!opts->addr.port) {
> -                       if (opts->ahmac) {
> +               if (!addr_info->port) {
> +                       if (!echo) {
>                                 put_unaligned_be64(opts->ahmac, ptr);
>                                 ptr += 2;
>                         }
>                 } else {
> -                       u16 port = ntohs(opts->addr.port);
> +                       u16 port = ntohs(addr_info->port);
>
> -                       if (opts->ahmac) {
> +                       if (!echo) {
>                                 u8 *bptr = (u8 *)ptr;
>
>                                 put_unaligned_be16(port, bptr);
> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>                                 bptr += 8;
>                                 put_unaligned_be16(TCPOPT_NOP << 8 |
>                                                    TCPOPT_NOP, bptr);
> -
>                                 ptr += 3;
>                         } else {
>                                 put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..a62d4a5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
>         lockdep_assert_held(&msk->pm.lock);
>
> -       if (add_addr) {
> +       if (add_addr &
> +           (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>                 pr_warn("addr_signal error, add_addr=%d", add_addr);
>                 return -EINVAL;
>         }
> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
>  /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> +                             struct mptcp_addr_info *daddr, u8 *add_addr)
>  {
> -       u8 add_addr;
> -       int ret = false;
> -
>         spin_lock_bh(&msk->pm.lock);
>
> -       /* double check after the lock is acquired */
> -       if (!mptcp_pm_should_add_signal(msk))
> -               goto out_unlock;
> -
> -       *echo = mptcp_pm_should_add_signal_echo(msk);
> -       *port = mptcp_pm_should_add_signal_port(msk);
> -
> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> -               goto out_unlock;
> -
>         *saddr = msk->pm.local;
> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
> -       ret = true;
> +       *daddr = msk->pm.remote;
> +       *add_addr = msk->pm.addr_signal;
>
> -out_unlock:
>         spin_unlock_bh(&msk->pm.lock);
> -       return ret;
> +
> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>  }
>
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0b0ec0..90fb532 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -22,10 +22,11 @@
>  #define OPTION_MPTCP_MPJ_SYNACK        BIT(4)
>  #define OPTION_MPTCP_MPJ_ACK   BIT(5)
>  #define OPTION_MPTCP_ADD_ADDR  BIT(6)
> -#define OPTION_MPTCP_RM_ADDR   BIT(7)
> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> -#define OPTION_MPTCP_PRIO      BIT(9)
> -#define OPTION_MPTCP_RST       BIT(10)
> +#define OPTION_MPTCP_ADD_ECHO  BIT(7)
> +#define OPTION_MPTCP_RM_ADDR   BIT(8)
> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> +#define OPTION_MPTCP_PRIO      BIT(10)
> +#define OPTION_MPTCP_RST       BIT(11)
>
>  /* MPTCP option subtypes */
>  #define MPTCPOPT_MP_CAPABLE    0
> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>  }
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> +                             struct mptcp_addr_info *daddr, u8 *add_addr);
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>                              struct mptcp_rm_list *rm_list);
>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> --
> 1.8.3.1
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-06-21  8:29 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18  8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-18  8:18 ` [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-18  8:18 ` [PATCH v4 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
2021-06-18  8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-18 11:20   ` Geliang Tang
2021-06-21  3:51     ` Yonglong Li
2021-06-21  6:42       ` Geliang Tang
2021-06-21  7:15         ` Yonglong Li
2021-06-21  7:39           ` Geliang Tang
2021-06-21  7:49             ` Yonglong Li
2021-06-21  8:06               ` Geliang Tang
2021-06-21  7:42   ` Geliang Tang
2021-06-21  7:51     ` Yonglong Li
2021-06-21  8:29   ` Geliang Tang
2021-06-18  8:18 ` [PATCH v4 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).