mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process
@ 2021-06-17  9:14 Yonglong Li
  2021-06-17  9:14 ` [PATCH v3 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Yonglong Li @ 2021-06-17  9:14 UTC (permalink / raw)
  To: mptcp
  Cc: pabeni, matthieu.baerts, mathew.j.martineau, geliangtang, 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.

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] 11+ messages in thread

* [PATCH v3 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
  2021-06-17  9:14 [PATCH v3 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
@ 2021-06-17  9:14 ` Yonglong Li
  2021-06-17 21:06   ` Mat Martineau
  2021-06-17  9:14 ` [PATCH v3 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Yonglong Li @ 2021-06-17  9:14 UTC (permalink / raw)
  To: mptcp
  Cc: pabeni, matthieu.baerts, mathew.j.martineau, geliangtang, 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..611bb2c7 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_RM_ADDR_SIGNAL);
+	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] 11+ messages in thread

* [PATCH v3 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
  2021-06-17  9:14 [PATCH v3 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
  2021-06-17  9:14 ` [PATCH v3 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
@ 2021-06-17  9:14 ` Yonglong Li
  2021-06-17  9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
  2021-06-17  9:14 ` [PATCH v3 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
  3 siblings, 0 replies; 11+ messages in thread
From: Yonglong Li @ 2021-06-17  9:14 UTC (permalink / raw)
  To: mptcp
  Cc: pabeni, matthieu.baerts, mathew.j.martineau, geliangtang, 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 611bb2c7..74be6d7 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] 11+ messages in thread

* [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-17  9:14 [PATCH v3 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
  2021-06-17  9:14 ` [PATCH v3 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
  2021-06-17  9:14 ` [PATCH v3 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
@ 2021-06-17  9:14 ` Yonglong Li
  2021-06-17 12:37   ` Geliang Tang
                     ` (2 more replies)
  2021-06-17  9:14 ` [PATCH v3 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
  3 siblings, 3 replies; 11+ messages in thread
From: Yonglong Li @ 2021-06-17  9:14 UTC (permalink / raw)
  To: mptcp
  Cc: pabeni, matthieu.baerts, mathew.j.martineau, geliangtang, 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  | 161 +++++++++++++++++++++++++++++++++------------------
 net/mptcp/pm.c       |  30 +++-------
 net/mptcp/protocol.h |  13 +++--
 3 files changed, 122 insertions(+), 82 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 1aec016..3ecf2c6 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -655,43 +655,72 @@ 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;
+	int ret = false;
+	u8 add_addr, flags;
 	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)
-		return false;
-
-	*size = len;
-	if (drop_other_suboptions)
-		*size -= opt_size;
-	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
-	if (!echo) {
+	if (!mptcp_pm_should_add_signal(msk))
+		goto out;
+
+	*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 && mptcp_pm_should_add_signal_addr(msk))
+			goto add_addr;
+		else if (remaining < len)
+			goto out;
+		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)) {
+add_addr:
+		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)
+			goto out;
+		*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));
 
-	return true;
+	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);
+	ret = true;
+
+out:
+	return ret;
 }
 
 static bool mptcp_established_options_rm_addr(struct sock *sk,
@@ -1230,21 +1259,18 @@ 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;
+		u8 echo = 0;
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 		if (opts->addr.family == AF_INET6)
 			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
 #endif
 
+		len += sizeof(opts->ahmac);
+
 		if (opts->addr.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) {
@@ -1259,30 +1285,55 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 #endif
 
 		if (!opts->addr.port) {
-			if (opts->ahmac) {
-				put_unaligned_be64(opts->ahmac, ptr);
-				ptr += 2;
-			}
+			put_unaligned_be64(opts->ahmac, ptr);
+			ptr += 2;
 		} else {
 			u16 port = ntohs(opts->addr.port);
+			u8 *bptr = (u8 *)ptr;
 
-			if (opts->ahmac) {
-				u8 *bptr = (u8 *)ptr;
+			put_unaligned_be16(port, bptr);
+			bptr += 2;
+			put_unaligned_be64(opts->ahmac, bptr);
+			bptr += 8;
+			put_unaligned_be16(TCPOPT_NOP << 8 |
+					   TCPOPT_NOP, bptr);
 
-				put_unaligned_be16(port, bptr);
-				bptr += 2;
-				put_unaligned_be64(opts->ahmac, bptr);
-				bptr += 8;
-				put_unaligned_be16(TCPOPT_NOP << 8 |
-						   TCPOPT_NOP, bptr);
+			ptr += 3;
+		}
+	}
 
-				ptr += 3;
-			} else {
-				put_unaligned_be32(port << 16 |
-						   TCPOPT_NOP << 8 |
-						   TCPOPT_NOP, ptr);
-				ptr += 1;
-			}
+	if (OPTION_MPTCP_ADD_ECHO & opts->suboptions) {
+		u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
+		u8 echo = MPTCP_ADDR_ECHO;
+
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+		if (opts->remote.family == AF_INET6)
+			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+#endif
+
+		if (opts->remote.port)
+			len += TCPOLEN_MPTCP_PORT_LEN;
+
+		*ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
+				      len, echo, opts->remote.id);
+		if (opts->remote.family == AF_INET) {
+			memcpy((u8 *)ptr, (u8 *)&opts->remote.addr.s_addr, 4);
+			ptr += 1;
+		}
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+		else if (opts->remote.family == AF_INET6) {
+			memcpy((u8 *)ptr, opts->remote.addr6.s6_addr, 16);
+			ptr += 4;
+		}
+#endif
+
+		if (opts->remote.port) {
+			u16 port = ntohs(opts->remote.port);
+
+			put_unaligned_be32(port << 16 |
+					   TCPOPT_NOP << 8 |
+					   TCPOPT_NOP, ptr);
+			ptr += 1;
 		}
 	}
 
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 74be6d7..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_RM_ADDR_SIGNAL);
-	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] 11+ messages in thread

* [PATCH v3 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
  2021-06-17  9:14 [PATCH v3 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-17  9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-17  9:14 ` Yonglong Li
  3 siblings, 0 replies; 11+ messages in thread
From: Yonglong Li @ 2021-06-17  9:14 UTC (permalink / raw)
  To: mptcp
  Cc: pabeni, matthieu.baerts, mathew.j.martineau, geliangtang, 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] 11+ messages in thread

* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-17  9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-17 12:37   ` Geliang Tang
  2021-06-18  1:10     ` Yonglong Li
  2021-06-17 19:22   ` kernel test robot
  2021-06-18  0:25   ` Mat Martineau
  2 siblings, 1 reply; 11+ messages in thread
From: Geliang Tang @ 2021-06-17 12:37 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, Paolo Abeni, Matthieu Baerts, Mat Martineau

Hi Yonglong,

Thanks for this patch set.

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月17日周四 下午5:15写道:
>
> 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  | 161 +++++++++++++++++++++++++++++++++------------------
>  net/mptcp/pm.c       |  30 +++-------
>  net/mptcp/protocol.h |  13 +++--
>  3 files changed, 122 insertions(+), 82 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..3ecf2c6 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,43 +655,72 @@ 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;
> +       int ret = false;
> +       u8 add_addr, flags;
>         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)
> -               return false;
> -
> -       *size = len;
> -       if (drop_other_suboptions)
> -               *size -= opt_size;
> -       opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> -       if (!echo) {
> +       if (!mptcp_pm_should_add_signal(msk))
> +               goto out;
> +
> +       *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 && mptcp_pm_should_add_signal_addr(msk))
> +                       goto add_addr;
> +               else if (remaining < len)
> +                       goto out;
> +               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)) {
> +add_addr:
> +               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)
> +                       goto out;
> +               *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);
>         }

There are some duplicate codes here between the
mptcp_pm_should_add_signal_echo(msk) trunk and the
mptcp_pm_should_add_signal_addr(msk) trunk, could you please simply them
into one trunk?

> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>
> -       return true;
> +       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);
> +       ret = true;
> +
> +out:
> +       return ret;
>  }
>
>  static bool mptcp_established_options_rm_addr(struct sock *sk,
> @@ -1230,21 +1259,18 @@ 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;
> +               u8 echo = 0;
>
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>                 if (opts->addr.family == AF_INET6)
>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>  #endif
>
> +               len += sizeof(opts->ahmac);
> +
>                 if (opts->addr.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) {
> @@ -1259,30 +1285,55 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>  #endif
>
>                 if (!opts->addr.port) {
> -                       if (opts->ahmac) {
> -                               put_unaligned_be64(opts->ahmac, ptr);
> -                               ptr += 2;
> -                       }
> +                       put_unaligned_be64(opts->ahmac, ptr);
> +                       ptr += 2;
>                 } else {
>                         u16 port = ntohs(opts->addr.port);
> +                       u8 *bptr = (u8 *)ptr;
>
> -                       if (opts->ahmac) {
> -                               u8 *bptr = (u8 *)ptr;
> +                       put_unaligned_be16(port, bptr);
> +                       bptr += 2;
> +                       put_unaligned_be64(opts->ahmac, bptr);
> +                       bptr += 8;
> +                       put_unaligned_be16(TCPOPT_NOP << 8 |
> +                                          TCPOPT_NOP, bptr);
>
> -                               put_unaligned_be16(port, bptr);
> -                               bptr += 2;
> -                               put_unaligned_be64(opts->ahmac, bptr);
> -                               bptr += 8;
> -                               put_unaligned_be16(TCPOPT_NOP << 8 |
> -                                                  TCPOPT_NOP, bptr);
> +                       ptr += 3;
> +               }
> +       }
>
> -                               ptr += 3;
> -                       } else {
> -                               put_unaligned_be32(port << 16 |
> -                                                  TCPOPT_NOP << 8 |
> -                                                  TCPOPT_NOP, ptr);
> -                               ptr += 1;
> -                       }
> +       if (OPTION_MPTCP_ADD_ECHO & opts->suboptions) {
> +               u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> +               u8 echo = MPTCP_ADDR_ECHO;
> +
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +               if (opts->remote.family == AF_INET6)
> +                       len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +#endif
> +
> +               if (opts->remote.port)
> +                       len += TCPOLEN_MPTCP_PORT_LEN;
> +
> +               *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> +                                     len, echo, opts->remote.id);
> +               if (opts->remote.family == AF_INET) {
> +                       memcpy((u8 *)ptr, (u8 *)&opts->remote.addr.s_addr, 4);
> +                       ptr += 1;
> +               }
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +               else if (opts->remote.family == AF_INET6) {
> +                       memcpy((u8 *)ptr, opts->remote.addr6.s6_addr, 16);
> +                       ptr += 4;
> +               }
> +#endif
> +
> +               if (opts->remote.port) {
> +                       u16 port = ntohs(opts->remote.port);
> +
> +                       put_unaligned_be32(port << 16 |
> +                                          TCPOPT_NOP << 8 |
> +                                          TCPOPT_NOP, ptr);
> +                       ptr += 1;
>                 }
>         }

And the same here between the OPTION_MPTCP_ADD_ADDR trunk and the
OPTION_MPTCP_ADD_ECHO trunk.

Thanks.
-Geliang

>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 74be6d7..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_RM_ADDR_SIGNAL);
> -       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] 11+ messages in thread

* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-17  9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
  2021-06-17 12:37   ` Geliang Tang
@ 2021-06-17 19:22   ` kernel test robot
  2021-06-18  0:25   ` Mat Martineau
  2 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2021-06-17 19:22 UTC (permalink / raw)
  To: Yonglong Li, mptcp
  Cc: kbuild-all, clang-built-linux, pabeni, matthieu.baerts,
	mathew.j.martineau, geliangtang, Yonglong Li

[-- Attachment #1: Type: text/plain, Size: 9971 bytes --]

Hi Yonglong,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mptcp/export]
[also build test WARNING on linus/master v5.13-rc6 next-20210617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Yonglong-Li/mptcp-fix-conflicts-when-using-pm-add_signal-in-ADD_ADDR-echo-and-RM_ADDR-process/20210617-171559
base:   https://github.com/multipath-tcp/mptcp_net-next.git export
config: x86_64-randconfig-a015-20210617 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/dcb008513c667a57c48dd885599f2d760c8cf7eb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Yonglong-Li/mptcp-fix-conflicts-when-using-pm-add_signal-in-ADD_ADDR-echo-and-RM_ADDR-process/20210617-171559
        git checkout dcb008513c667a57c48dd885599f2d760c8cf7eb
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/mptcp/options.c:567:21: warning: parameter 'remaining' set but not used [-Wunused-but-set-parameter]
                                             unsigned int remaining,
                                                          ^
>> net/mptcp/options.c:698:9: warning: variable 'flags' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           } else if (mptcp_pm_should_add_signal_addr(msk)) {
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:56:28: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:58:30: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   net/mptcp/options.c:726:34: note: uninitialized use occurs here
           WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
                                           ^~~~~
   include/asm-generic/rwonce.h:61:18: note: expanded from macro 'WRITE_ONCE'
           __WRITE_ONCE(x, val);                                           \
                           ^~~
   include/asm-generic/rwonce.h:55:33: note: expanded from macro '__WRITE_ONCE'
           *(volatile typeof(x) *)&(x) = (val);                            \
                                          ^~~
   net/mptcp/options.c:698:9: note: remove the 'if' if its condition is always true
           } else if (mptcp_pm_should_add_signal_addr(msk)) {
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:56:23: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                         ^
   net/mptcp/options.c:669:20: note: initialize the variable 'flags' to silence this warning
           u8 add_addr, flags;
                             ^
                              = '\0'
   2 warnings generated.


vim +698 net/mptcp/options.c

   563	
   564	static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
   565						  bool snd_data_fin_enable,
   566						  unsigned int *size,
 > 567						  unsigned int remaining,
   568						  struct mptcp_out_options *opts)
   569	{
   570		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
   571		struct mptcp_sock *msk = mptcp_sk(subflow->conn);
   572		unsigned int dss_size = 0;
   573		struct mptcp_ext *mpext;
   574		unsigned int ack_size;
   575		bool ret = false;
   576		u64 ack_seq;
   577	
   578		opts->csum_reqd = READ_ONCE(msk->csum_enabled);
   579		mpext = skb ? mptcp_get_ext(skb) : NULL;
   580	
   581		if (!skb || (mpext && mpext->use_map) || snd_data_fin_enable) {
   582			unsigned int map_size = TCPOLEN_MPTCP_DSS_BASE + TCPOLEN_MPTCP_DSS_MAP64;
   583	
   584			if (mpext) {
   585				if (opts->csum_reqd)
   586					map_size += TCPOLEN_MPTCP_DSS_CHECKSUM;
   587	
   588				opts->ext_copy = *mpext;
   589			}
   590	
   591			remaining -= map_size;
   592			dss_size = map_size;
   593			if (skb && snd_data_fin_enable)
   594				mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
   595			ret = true;
   596		}
   597	
   598		/* passive sockets msk will set the 'can_ack' after accept(), even
   599		 * if the first subflow may have the already the remote key handy
   600		 */
   601		opts->ext_copy.use_ack = 0;
   602		if (!READ_ONCE(msk->can_ack)) {
   603			*size = ALIGN(dss_size, 4);
   604			return ret;
   605		}
   606	
   607		ack_seq = READ_ONCE(msk->ack_seq);
   608		if (READ_ONCE(msk->use_64bit_ack)) {
   609			ack_size = TCPOLEN_MPTCP_DSS_ACK64;
   610			opts->ext_copy.data_ack = ack_seq;
   611			opts->ext_copy.ack64 = 1;
   612		} else {
   613			ack_size = TCPOLEN_MPTCP_DSS_ACK32;
   614			opts->ext_copy.data_ack32 = (uint32_t)ack_seq;
   615			opts->ext_copy.ack64 = 0;
   616		}
   617		opts->ext_copy.use_ack = 1;
   618		WRITE_ONCE(msk->old_wspace, __mptcp_space((struct sock *)msk));
   619	
   620		/* Add kind/length/subtype/flag overhead if mapping is not populated */
   621		if (dss_size == 0)
   622			ack_size += TCPOLEN_MPTCP_DSS_BASE;
   623	
   624		dss_size += ack_size;
   625	
   626		*size = ALIGN(dss_size, 4);
   627		return true;
   628	}
   629	
   630	static u64 add_addr_generate_hmac(u64 key1, u64 key2,
   631					  struct mptcp_addr_info *addr)
   632	{
   633		u16 port = ntohs(addr->port);
   634		u8 hmac[SHA256_DIGEST_SIZE];
   635		u8 msg[19];
   636		int i = 0;
   637	
   638		msg[i++] = addr->id;
   639		if (addr->family == AF_INET) {
   640			memcpy(&msg[i], &addr->addr.s_addr, 4);
   641			i += 4;
   642		}
   643	#if IS_ENABLED(CONFIG_MPTCP_IPV6)
   644		else if (addr->family == AF_INET6) {
   645			memcpy(&msg[i], &addr->addr6.s6_addr, 16);
   646			i += 16;
   647		}
   648	#endif
   649		msg[i++] = port >> 8;
   650		msg[i++] = port & 0xFF;
   651	
   652		mptcp_crypto_hmac_sha(key1, key2, msg, i, hmac);
   653	
   654		return get_unaligned_be64(&hmac[SHA256_DIGEST_SIZE - sizeof(u64)]);
   655	}
   656	
   657	static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *skb,
   658						       unsigned int *size,
   659						       unsigned int remaining,
   660						       struct mptcp_out_options *opts)
   661	{
   662		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
   663		struct mptcp_sock *msk = mptcp_sk(subflow->conn);
   664		bool drop_other_suboptions = false;
   665		unsigned int opt_size = *size;
   666		struct mptcp_addr_info remote;
   667		struct mptcp_addr_info local;
   668		int ret = false;
   669		u8 add_addr, flags;
   670		int len;
   671	
   672		if (!mptcp_pm_should_add_signal(msk))
   673			goto out;
   674	
   675		*size = 0;
   676		mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
   677		if (mptcp_pm_should_add_signal_echo(msk)) {
   678			if (skb && skb_is_tcp_pure_ack(skb)) {
   679				pr_debug("drop other suboptions");
   680				opts->suboptions = 0;
   681				opts->ext_copy.use_ack = 0;
   682				opts->ext_copy.use_map = 0;
   683				remaining += opt_size;
   684				drop_other_suboptions = true;
   685			}
   686			len = mptcp_add_addr_len(remote.family, true, !!remote.port);
   687			if (remaining < len && mptcp_pm_should_add_signal_addr(msk))
   688				goto add_addr;
   689			else if (remaining < len)
   690				goto out;
   691			remaining -= len;
   692			*size += len;
   693			opts->remote = remote;
   694			flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
   695			opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
   696			pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
   697				 opts->remote.id, ntohs(opts->remote.port), add_addr);
 > 698		} else if (mptcp_pm_should_add_signal_addr(msk)) {
   699	add_addr:
   700			if ((local.family == AF_INET6 || local.port) && skb &&
   701			    skb_is_tcp_pure_ack(skb)) {
   702				pr_debug("drop other suboptions");
   703				opts->suboptions = 0;
   704				opts->ext_copy.use_ack = 0;
   705				opts->ext_copy.use_map = 0;
   706				remaining += opt_size;
   707				drop_other_suboptions = true;
   708			}
   709			len = mptcp_add_addr_len(local.family, false, !!local.port);
   710			if (remaining < len)
   711				goto out;
   712			*size += len;
   713			opts->addr = local;
   714			opts->ahmac = add_addr_generate_hmac(msk->local_key,
   715							     msk->remote_key,
   716							     &opts->addr);
   717			opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
   718			flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
   719			pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
   720				 opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
   721		}
   722	
   723		if (drop_other_suboptions)
   724			*size -= opt_size;
   725		spin_lock_bh(&msk->pm.lock);
   726		WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
   727		spin_unlock_bh(&msk->pm.lock);
   728		ret = true;
   729	
   730	out:
   731		return ret;
   732	}
   733	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31950 bytes --]

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

* Re: [PATCH v3 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
  2021-06-17  9:14 ` [PATCH v3 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
@ 2021-06-17 21:06   ` Mat Martineau
  0 siblings, 0 replies; 11+ messages in thread
From: Mat Martineau @ 2021-06-17 21:06 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, pabeni, matthieu.baerts, geliangtang

On Thu, 17 Jun 2021, Yonglong Li wrote:

> 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..611bb2c7 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_RM_ADDR_SIGNAL);

Thanks for your reply for my comments in the v2 of this patch. I did 
misunderstand that the clearing of MPTCP_ADD_ADDR_ECHO here was 
intentional.

Still, I'd prefer to have it written

~(BIT(MPTCP_ADD_ADDR_SIGNAL | BIT(MPTCP_ADD_ADDR_ECHO))

so it more obviously lists the bits to be cleared. Also can't assume that 
other bits in msk->pm.addr_signal will remain unused forever.

-Mat

> +	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
>
>

--
Mat Martineau
Intel

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

* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-17  9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
  2021-06-17 12:37   ` Geliang Tang
  2021-06-17 19:22   ` kernel test robot
@ 2021-06-18  0:25   ` Mat Martineau
  2021-06-18  1:24     ` Yonglong Li
  2 siblings, 1 reply; 11+ messages in thread
From: Mat Martineau @ 2021-06-18  0:25 UTC (permalink / raw)
  To: Yonglong Li; +Cc: mptcp, pabeni, matthieu.baerts, geliangtang

On Thu, 17 Jun 2021, Yonglong Li wrote:

> 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  | 161 +++++++++++++++++++++++++++++++++------------------
> net/mptcp/pm.c       |  30 +++-------
> net/mptcp/protocol.h |  13 +++--
> 3 files changed, 122 insertions(+), 82 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..3ecf2c6 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,43 +655,72 @@ 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;
> +	int ret = false;
> +	u8 add_addr, flags;
> 	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)
> -		return false;
> -
> -	*size = len;
> -	if (drop_other_suboptions)
> -		*size -= opt_size;
> -	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> -	if (!echo) {
> +	if (!mptcp_pm_should_add_signal(msk))
> +		goto out;

Hi Yonglong, thanks for revising.

Instead of the goto here, just "return true;".

> +
> +	*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 && mptcp_pm_should_add_signal_addr(msk))
> +			goto add_addr;

This goto isn't quite right. It jumps below with opts and remaining 
already modified, and may end up modifying 'remaining' again.

Would be better to separate the logic for sending echo-vs-signal, so the 
goto isn't necessary.

> +		else if (remaining < len)
> +			goto out;
> +		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)) {
> +add_addr:
> +		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)
> +			goto out;
> +		*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));
>
> -	return true;
> +	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);

This would set bits in msk->pm.addr_signal rather than clear them. Did you 
intend '&' instead of '|'?

As the kbuild bot noted, 'flags' can be uninitialized. That code path is 
not expected and shouldn't happen, but since the pm lock is not held the 
whole time the code should handle concurrent changes to 
msk->pm.addr_signal. Could initialize flags to 0 and only 
lock/write/unlock if flags is nonzero.

> +	ret = true;
> +
> +out:
> +	return ret;

Since the return is the only thing after the label, better to not use 
'goto' and use return statements where needed in the code above.

-Mat


> }
>
> static bool mptcp_established_options_rm_addr(struct sock *sk,
> @@ -1230,21 +1259,18 @@ 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;
> +		u8 echo = 0;
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> 		if (opts->addr.family == AF_INET6)
> 			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> #endif
>
> +		len += sizeof(opts->ahmac);
> +
> 		if (opts->addr.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) {
> @@ -1259,30 +1285,55 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> #endif
>
> 		if (!opts->addr.port) {
> -			if (opts->ahmac) {
> -				put_unaligned_be64(opts->ahmac, ptr);
> -				ptr += 2;
> -			}
> +			put_unaligned_be64(opts->ahmac, ptr);
> +			ptr += 2;
> 		} else {
> 			u16 port = ntohs(opts->addr.port);
> +			u8 *bptr = (u8 *)ptr;
>
> -			if (opts->ahmac) {
> -				u8 *bptr = (u8 *)ptr;
> +			put_unaligned_be16(port, bptr);
> +			bptr += 2;
> +			put_unaligned_be64(opts->ahmac, bptr);
> +			bptr += 8;
> +			put_unaligned_be16(TCPOPT_NOP << 8 |
> +					   TCPOPT_NOP, bptr);
>
> -				put_unaligned_be16(port, bptr);
> -				bptr += 2;
> -				put_unaligned_be64(opts->ahmac, bptr);
> -				bptr += 8;
> -				put_unaligned_be16(TCPOPT_NOP << 8 |
> -						   TCPOPT_NOP, bptr);
> +			ptr += 3;
> +		}
> +	}
>
> -				ptr += 3;
> -			} else {
> -				put_unaligned_be32(port << 16 |
> -						   TCPOPT_NOP << 8 |
> -						   TCPOPT_NOP, ptr);
> -				ptr += 1;
> -			}
> +	if (OPTION_MPTCP_ADD_ECHO & opts->suboptions) {
> +		u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> +		u8 echo = MPTCP_ADDR_ECHO;
> +
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +		if (opts->remote.family == AF_INET6)
> +			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +#endif
> +
> +		if (opts->remote.port)
> +			len += TCPOLEN_MPTCP_PORT_LEN;
> +
> +		*ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> +				      len, echo, opts->remote.id);
> +		if (opts->remote.family == AF_INET) {
> +			memcpy((u8 *)ptr, (u8 *)&opts->remote.addr.s_addr, 4);
> +			ptr += 1;
> +		}
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +		else if (opts->remote.family == AF_INET6) {
> +			memcpy((u8 *)ptr, opts->remote.addr6.s6_addr, 16);
> +			ptr += 4;
> +		}
> +#endif
> +
> +		if (opts->remote.port) {
> +			u16 port = ntohs(opts->remote.port);
> +
> +			put_unaligned_be32(port << 16 |
> +					   TCPOPT_NOP << 8 |
> +					   TCPOPT_NOP, ptr);
> +			ptr += 1;
> 		}
> 	}
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 74be6d7..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_RM_ADDR_SIGNAL);
> -	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
>
>
>

--
Mat Martineau
Intel

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

* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-17 12:37   ` Geliang Tang
@ 2021-06-18  1:10     ` Yonglong Li
  0 siblings, 0 replies; 11+ messages in thread
From: Yonglong Li @ 2021-06-18  1:10 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp

Hi Geliang,

Thanks for your review. I will simply the code and send v4 patch.

On 2021/6/17 20:37, Geliang Tang wrote:
>>                                                      &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);
>>         }
> There are some duplicate codes here between the
> mptcp_pm_should_add_signal_echo(msk) trunk and the
> mptcp_pm_should_add_signal_addr(msk) trunk, could you please simply them
> into one trunk?
> 

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

* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  2021-06-18  0:25   ` Mat Martineau
@ 2021-06-18  1:24     ` Yonglong Li
  0 siblings, 0 replies; 11+ messages in thread
From: Yonglong Li @ 2021-06-18  1:24 UTC (permalink / raw)
  To: Mat Martineau; +Cc: mptcp



On 2021/6/18 8:25, Mat Martineau wrote:
> 
> This goto isn't quite right. It jumps below with opts and remaining already modified, and may end up modifying 'remaining' again.
> 
> Would be better to separate the logic for sending echo-vs-signal, so the goto isn't necessary.

Thanks for your review. The goto logic is not right indeed. I will separate the logic for sending echo-vs-signal

> 
>> +        else if (remaining < len)
>> +            goto out;
>> +        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)) {
>> +add_addr:
>> +        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)
>> +            goto out;
>> +        *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));
>>
>> -    return true;
>> +    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);
> 
> This would set bits in msk->pm.addr_signal rather than clear them. Did you intend '&' instead of '|'?

Sorry for this mistake. :(

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

end of thread, other threads:[~2021-06-18  1:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17  9:14 [PATCH v3 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-17  9:14 ` [PATCH v3 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-17 21:06   ` Mat Martineau
2021-06-17  9:14 ` [PATCH v3 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
2021-06-17  9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-17 12:37   ` Geliang Tang
2021-06-18  1:10     ` Yonglong Li
2021-06-17 19:22   ` kernel test robot
2021-06-18  0:25   ` Mat Martineau
2021-06-18  1:24     ` Yonglong Li
2021-06-17  9:14 ` [PATCH v3 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).