[MPTCP][PATCH v9 mptcp-next 0/6] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[MPTCP][PATCH v9 mptcp-next 0/6] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[Geliang Tang]

From: Yonglong Li <liyonglong@chinatelecom.cn>

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: mptcp_pm_should_add_addr => 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;

v4->v5:
 - Patch 3: simple the code of 'mptcp_established_options_add_addr'

v5->v6:
 - Patch2: fix fails of 'mptcp_join.sh -t'. In mptcp_pm_add_addr_send_ack
   without MPTCP_ADD_ADDR_SIGNAL check so pure ack can not be sent for
   ADD_ADDR. That cause ADD_ADDR can not be sent in time.
 - Patch3: refactor some code according Geliang's suggestions.
 - Patch4: modify commit comment

v6->v7:
 - Patch1: use reverse xmas tree order for variables definition
 - Patch3: refactor some code according Geliang's suggestions.
 - add a Patch4: remove some double-check

v7->v8:
 - Patch1,2: exchange patch1 and patch2
 - Patch3: refactor some code according Geliang's suggestions.
 - remove patch "remove some double check", Geliang think it's unnecessary

v8->v9:
 - Keep mptcp_add_addr_len unchanged.
 - populate opts->local or opts->remote after the length check, don't
   populate both of them.
 - add back 'echo' and 'port' arguments of mptcp_pm_add_addr_signal().
 - add a new arguments drop_other_suboptions for
   mptcp_pm_add_addr_signal(), and do the drop_other_suboptions check in it.
 - drop other suboptions in mptcp_established_options_add_addr() after the
   length check.
 - split the drop_other_suboptions code into a new patch.
 - add a new selftest case.

Yonglong Li (6):
  mptcp: move drop_other_suboptions check under pm lock
  mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
  mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
  mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
  selftests: mptcp: add_addr and echo race test

 include/net/mptcp.h                           |  3 +-
 net/mptcp/options.c                           | 54 +++++++++---------
 net/mptcp/pm.c                                | 57 +++++++++++++------
 net/mptcp/pm_netlink.c                        | 10 ++--
 net/mptcp/protocol.h                          | 24 ++++----
 .../testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++
 6 files changed, 97 insertions(+), 66 deletions(-)

-- 
2.31.1

[MPTCP][PATCH v9 mptcp-next 1/6] mptcp: move drop_other_suboptions check under pm lock

[Geliang Tang]

From: Yonglong Li <liyonglong@chinatelecom.cn>

This patch moved the drop_other_suboptions check from
mptcp_established_options_add_addr() into mptcp_pm_add_addr_signal(), do
it under the PM lock to avoid the race between this check and
mptcp_pm_add_addr_signal().

For this, added a new parameter for mptcp_pm_add_addr_signal() to get
the drop_other_suboptions value. And drop the other suboptions after the
option length check if drop_other_suboptions is true.

Co-developed-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/options.c  | 24 ++++++++++--------------
 net/mptcp/pm.c       | 14 ++++++++++++--
 net/mptcp/protocol.h |  6 ++++--
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 4452455aef7f..dc09c853ed5b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -667,29 +667,25 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	bool port;
 	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)))
+	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, &opts->addr,
+		    &echo, &port, &drop_other_suboptions))
 		return false;
 
+	if (drop_other_suboptions)
+		remaining += opt_size;
 	len = mptcp_add_addr_len(opts->addr.family, echo, port);
 	if (remaining < len)
 		return false;
 
 	*size = len;
-	if (drop_other_suboptions)
+	if (drop_other_suboptions) {
+		pr_debug("drop other suboptions");
+		opts->suboptions = 0;
+		opts->ext_copy.use_ack = 0;
+		opts->ext_copy.use_map = 0;
 		*size -= opt_size;
+	}
 	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
 	if (!echo) {
 		opts->ahmac = add_addr_generate_hmac(msk->local_key,
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 639271e09604..c0a2c55008e3 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -249,8 +249,10 @@ 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)
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
+			      unsigned int opt_size, unsigned int remaining,
+			      struct mptcp_addr_info *saddr, bool *echo,
+			      bool *port, bool *drop_other_suboptions)
 {
 	int ret = false;
 
@@ -260,6 +262,14 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 	if (!mptcp_pm_should_add_signal(msk))
 		goto out_unlock;
 
+	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)) {
+		remaining += opt_size;
+		*drop_other_suboptions = true;
+	}
+
 	*echo = mptcp_pm_should_add_signal_echo(msk);
 	*port = mptcp_pm_should_add_signal_port(msk);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 0f0c026c5f8b..96bbbb9698db 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -776,8 +776,10 @@ 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);
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
+			      unsigned int opt_size, unsigned int remaining,
+			      struct mptcp_addr_info *saddr, bool *echo,
+			      bool *port, bool *drop_other_suboptions);
 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);
-- 
2.31.1

[MPTCP][PATCH v9 mptcp-next 2/6] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate

[Geliang Tang]

From: Yonglong Li <liyonglong@chinatelecom.cn>

Use MPTCP_ADD_ADDR_SIGNAL only for the action of sending ADD_ADDR, and
use MPTCP_ADD_ADDR_ECHO only for the action of sending ADD_ADDR echo.

Use msk->pm.local to save the announced ADD_ADDR address only, and reuse
msk->pm.remote to save the announced ADD_ADDR_ECHO address.

To prepare for the next patch.

Co-developed-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/pm.c         | 16 ++++++++++------
 net/mptcp/pm_netlink.c |  4 ++--
 net/mptcp/protocol.h   |  6 ++++++
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index c0a2c55008e3..311336ce1247 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -18,19 +18,23 @@ 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);
 
-	if (add_addr) {
-		pr_warn("addr_signal error, add_addr=%d", add_addr);
+	if (add_addr &
+	    (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
+		pr_warn("addr_signal error, add_addr=%d, echo=%d", add_addr, echo);
 		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)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d2591ebf01d9..4ad4c8ae93a4 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 96bbbb9698db..023a6903f2c8 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -729,6 +729,12 @@ void mptcp_event_addr_announced(const struct mptcp_sock *msk, const struct mptcp
 void mptcp_event_addr_removed(const struct mptcp_sock *msk, u8 id);
 
 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);
 }
-- 
2.31.1

[MPTCP][PATCH v9 mptcp-next 3/6] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other

[Geliang Tang]

From: Yonglong Li <liyonglong@chinatelecom.cn>

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

Co-developed-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/pm.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 311336ce1247..046ad6ad6692 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -259,6 +259,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 			      bool *port, bool *drop_other_suboptions)
 {
 	int ret = false;
+	u8 add_addr;
 
 	spin_lock_bh(&msk->pm.lock);
 
@@ -281,7 +282,11 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 		goto out_unlock;
 
 	*saddr = msk->pm.local;
-	WRITE_ONCE(msk->pm.addr_signal, 0);
+	if (*echo)
+		add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
+	else
+		add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
+	WRITE_ONCE(msk->pm.addr_signal, add_addr);
 	ret = true;
 
 out_unlock:
@@ -293,6 +298,7 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list)
 {
 	int ret = false, len;
+	u8 rm_addr;
 
 	spin_lock_bh(&msk->pm.lock);
 
@@ -300,16 +306,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:
-- 
2.31.1

[MPTCP][PATCH v9 mptcp-next 4/6] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal

[Geliang Tang]

From: Yonglong Li <liyonglong@chinatelecom.cn>

According to the MPTCP_ADD_ADDR_SIGNAL or MPTCP_ADD_ADDR_ECHO flag, build
the ADD_ADDR/ADD_ADDR_ECHO option.

Add a new member remote in struct mptcp_out_options for the ADD_ADDR_ECHO,
and rename addr to local for the ADD_ADDR only.

In mptcp_pm_add_addr_signal(), use opts->remote or opts->local to save the
announced ADD_ADDR or ADD_ADDR_ECHO address. And in mptcp_write_options(),
put this saved address into the ADD_ADDR/ADD_ADDR_ECHO option.

Co-developed-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 include/net/mptcp.h  |  3 ++-
 net/mptcp/options.c  | 32 ++++++++++++++++++--------------
 net/mptcp/pm.c       | 14 +++++++++-----
 net/mptcp/protocol.h |  2 +-
 4 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 8b5af683a818..d0b9e4a7121f 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -61,7 +61,8 @@ struct mptcp_out_options {
 	u64 sndr_key;
 	u64 rcvr_key;
 	u64 ahmac;
-	struct mptcp_addr_info addr;
+	struct mptcp_addr_info local;
+	struct mptcp_addr_info remote;
 	struct mptcp_rm_list rm_list;
 	u8 join_id;
 	u8 backup;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index dc09c853ed5b..37ff15aeb2f7 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -665,16 +665,18 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	unsigned int opt_size = *size;
 	bool echo;
 	bool port;
+	u8 family;
 	int len;
 
 	if (!mptcp_pm_should_add_signal(msk) ||
-	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, &opts->addr,
+	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts,
 		    &echo, &port, &drop_other_suboptions))
 		return false;
 
 	if (drop_other_suboptions)
 		remaining += opt_size;
-	len = mptcp_add_addr_len(opts->addr.family, echo, port);
+	family = echo ? opts->remote.family : opts->local.family;
+	len = mptcp_add_addr_len(family, echo, port);
 	if (remaining < len)
 		return false;
 
@@ -690,10 +692,11 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	if (!echo) {
 		opts->ahmac = add_addr_generate_hmac(msk->local_key,
 						     msk->remote_key,
-						     &opts->addr);
+						     &opts->local);
 	}
-	pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
-		 opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
+	pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
+		 opts->local.id, ntohs(opts->local.port), opts->remote.id,
+		 ntohs(opts->remote.port), opts->ahmac, echo);
 
 	return true;
 }
@@ -1248,15 +1251,16 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 
 mp_capable_done:
 	if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
+		struct mptcp_addr_info *addr = opts->ahmac ? &opts->local : &opts->remote;
 		u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
 		u8 echo = MPTCP_ADDR_ECHO;
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-		if (opts->addr.family == AF_INET6)
+		if (addr->family == AF_INET6)
 			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
 #endif
 
-		if (opts->addr.port)
+		if (addr->port)
 			len += TCPOLEN_MPTCP_PORT_LEN;
 
 		if (opts->ahmac) {
@@ -1265,25 +1269,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 		}
 
 		*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->id);
+		if (addr->family == AF_INET) {
+			memcpy((u8 *)ptr, (u8 *)&addr->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->family == AF_INET6) {
+			memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
 			ptr += 4;
 		}
 #endif
 
-		if (!opts->addr.port) {
+		if (!addr->port) {
 			if (opts->ahmac) {
 				put_unaligned_be64(opts->ahmac, ptr);
 				ptr += 2;
 			}
 		} else {
-			u16 port = ntohs(opts->addr.port);
+			u16 port = ntohs(addr->port);
 
 			if (opts->ahmac) {
 				u8 *bptr = (u8 *)ptr;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 046ad6ad6692..10c9f44a1749 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -255,11 +255,12 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
 
 bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 			      unsigned int opt_size, unsigned int remaining,
-			      struct mptcp_addr_info *saddr, bool *echo,
+			      struct mptcp_out_options *opts, bool *echo,
 			      bool *port, bool *drop_other_suboptions)
 {
 	int ret = false;
 	u8 add_addr;
+	u8 family;
 
 	spin_lock_bh(&msk->pm.lock);
 
@@ -278,14 +279,17 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 	*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))
+	family = *echo ? msk->pm.remote.family : msk->pm.local.family;
+	if (remaining < mptcp_add_addr_len(family, *echo, *port))
 		goto out_unlock;
 
-	*saddr = msk->pm.local;
-	if (*echo)
+	if (*echo) {
+		opts->remote = msk->pm.remote;
 		add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
-	else
+	} else {
+		opts->local = msk->pm.local;
 		add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
+	}
 	WRITE_ONCE(msk->pm.addr_signal, add_addr);
 	ret = true;
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 023a6903f2c8..1993e39a31c0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -784,7 +784,7 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
 
 bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 			      unsigned int opt_size, unsigned int remaining,
-			      struct mptcp_addr_info *saddr, bool *echo,
+			      struct mptcp_out_options *opts, bool *echo,
 			      bool *port, bool *drop_other_suboptions);
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
-- 
2.31.1

[MPTCP][PATCH v9 mptcp-next 5/6] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT

[Geliang Tang]

From: Yonglong Li <liyonglong@chinatelecom.cn>

MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT are not necessary, we can get
these info from pm.local or pm.remote.

Drop mptcp_pm_should_add_signal_ipv6 and mptcp_pm_should_add_signal_port
too.

Co-developed-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/pm.c         | 12 ++++--------
 net/mptcp/pm_netlink.c |  6 ++----
 net/mptcp/protocol.h   | 12 ------------
 3 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 10c9f44a1749..d0f39e814d1d 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;
 }
@@ -268,16 +264,16 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 	if (!mptcp_pm_should_add_signal(msk))
 		goto out_unlock;
 
-	if ((mptcp_pm_should_add_signal_ipv6(msk) ||
-	     mptcp_pm_should_add_signal_port(msk) ||
-	     mptcp_pm_should_add_signal_echo(msk)) &&
+	if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
+	     ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
+	      (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
 	    skb && skb_is_tcp_pure_ack(skb)) {
 		remaining += opt_size;
 		*drop_other_suboptions = true;
 	}
 
 	*echo = mptcp_pm_should_add_signal_echo(msk);
-	*port = mptcp_pm_should_add_signal_port(msk);
+	*port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
 
 	family = *echo ? msk->pm.remote.family : msk->pm.local.family;
 	if (remaining < mptcp_add_addr_len(family, *echo, *port))
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 4ad4c8ae93a4..5694f51deee2 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -544,10 +544,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 1993e39a31c0..f8aa0f031ff7 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -178,8 +178,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,
 };
 
@@ -744,16 +742,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);
-- 
2.31.1

[MPTCP][PATCH v9 mptcp-next 6/6] selftests: mptcp: add_addr and echo race test

[Geliang Tang]

From: Yonglong Li <liyonglong@chinatelecom.cn>

This patch added an extra test for the singal_address_tests() to do the
ADD_ADDR and ADD_ADDR_ECHO race test.

Co-developed-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f02f4de2f3a0..8ec9f3408de1 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -918,6 +918,21 @@ signal_address_tests()
 	run_tests $ns1 $ns2 10.0.1.1
 	chk_join_nr "signal invalid addresses" 1 1 1
 	chk_add_nr 3 3
+
+	# signal addresses race test
+	reset
+	ip netns exec $ns1 ./pm_nl_ctl limits 4 4
+	ip netns exec $ns2 ./pm_nl_ctl limits 4 4
+	ip netns exec $ns1 ./pm_nl_ctl add 10.0.1.1 flags signal
+	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
+	ip netns exec $ns1 ./pm_nl_ctl add 10.0.3.1 flags signal
+	ip netns exec $ns1 ./pm_nl_ctl add 10.0.4.1 flags signal
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.1.2 flags signal
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 flags signal
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags signal
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.4.2 flags signal
+	run_tests $ns1 $ns2 10.0.1.1
+	chk_add_nr 4 4
 }
 
 link_failure_tests()
-- 
2.31.1

[MPTCP][PATCH v9 mptcp-next 0/6] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[Geliang Tang]

From: Yonglong Li <liyonglong@chinatelecom.cn>

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: mptcp_pm_should_add_addr => 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;

v4->v5:
 - Patch 3: simple the code of 'mptcp_established_options_add_addr'

v5->v6:
 - Patch2: fix fails of 'mptcp_join.sh -t'. In mptcp_pm_add_addr_send_ack
   without MPTCP_ADD_ADDR_SIGNAL check so pure ack can not be sent for
   ADD_ADDR. That cause ADD_ADDR can not be sent in time.
 - Patch3: refactor some code according Geliang's suggestions.
 - Patch4: modify commit comment

v6->v7:
 - Patch1: use reverse xmas tree order for variables definition
 - Patch3: refactor some code according Geliang's suggestions.
 - add a Patch4: remove some double-check

v7->v8:
 - Patch1,2: exchange patch1 and patch2
 - Patch3: refactor some code according Geliang's suggestions.
 - remove patch "remove some double check", Geliang think it's unnecessary

v8->v9:
 - Keep mptcp_add_addr_len unchanged.
 - populate opts->local or opts->remote after the length check, don't
   populate both of them.
 - add back 'echo' and 'port' arguments of mptcp_pm_add_addr_signal().
 - add a new arguments drop_other_suboptions for
   mptcp_pm_add_addr_signal(), and do the drop_other_suboptions check in it.
 - drop other suboptions in mptcp_established_options_add_addr() after the
   length check.
 - split the drop_other_suboptions code into a new patch.
 - add a new selftest case.

Yonglong Li (6):
  mptcp: move drop_other_suboptions check under pm lock
  mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
  mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
  mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
  mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
  selftests: mptcp: add_addr and echo race test

 include/net/mptcp.h                           |  3 +-
 net/mptcp/options.c                           | 54 +++++++++---------
 net/mptcp/pm.c                                | 57 +++++++++++++------
 net/mptcp/pm_netlink.c                        | 10 ++--
 net/mptcp/protocol.h                          | 24 ++++----
 .../testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++
 6 files changed, 97 insertions(+), 66 deletions(-)

-- 
2.31.1

Re: [MPTCP][PATCH v9 mptcp-next 0/6] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[Yonglong Li]

Hi Geliang,

The v9 LGTM. Thank you for your help and efforts.

On 2021/7/16 11:04, Geliang Tang wrote:
> From: Yonglong Li <liyonglong@chinatelecom.cn>
> 
> 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: mptcp_pm_should_add_addr => 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;
> 
> v4->v5:
>  - Patch 3: simple the code of 'mptcp_established_options_add_addr'
> 
> v5->v6:
>  - Patch2: fix fails of 'mptcp_join.sh -t'. In mptcp_pm_add_addr_send_ack
>    without MPTCP_ADD_ADDR_SIGNAL check so pure ack can not be sent for
>    ADD_ADDR. That cause ADD_ADDR can not be sent in time.
>  - Patch3: refactor some code according Geliang's suggestions.
>  - Patch4: modify commit comment
> 
> v6->v7:
>  - Patch1: use reverse xmas tree order for variables definition
>  - Patch3: refactor some code according Geliang's suggestions.
>  - add a Patch4: remove some double-check
> 
> v7->v8:
>  - Patch1,2: exchange patch1 and patch2
>  - Patch3: refactor some code according Geliang's suggestions.
>  - remove patch "remove some double check", Geliang think it's unnecessary
> 
> v8->v9:
>  - Keep mptcp_add_addr_len unchanged.
>  - populate opts->local or opts->remote after the length check, don't
>    populate both of them.
>  - add back 'echo' and 'port' arguments of mptcp_pm_add_addr_signal().
>  - add a new arguments drop_other_suboptions for
>    mptcp_pm_add_addr_signal(), and do the drop_other_suboptions check in it.
>  - drop other suboptions in mptcp_established_options_add_addr() after the
>    length check.
>  - split the drop_other_suboptions code into a new patch.
>  - add a new selftest case.
> 
> Yonglong Li (6):
>   mptcp: move drop_other_suboptions check under pm lock
>   mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
>   mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
>   mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
>   mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
>   selftests: mptcp: add_addr and echo race test
> 
>  include/net/mptcp.h                           |  3 +-
>  net/mptcp/options.c                           | 54 +++++++++---------
>  net/mptcp/pm.c                                | 57 +++++++++++++------
>  net/mptcp/pm_netlink.c                        | 10 ++--
>  net/mptcp/protocol.h                          | 24 ++++----
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++
>  6 files changed, 97 insertions(+), 66 deletions(-)
> 

-- 
Li YongLong

Re: [MPTCP][PATCH v9 mptcp-next 0/6] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[Geliang Tang]

Thanks, Yonglong,

I'll do more tests for v9. And please test v9 on your computer too.

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月16日周五 下午5:36写道：
>
>
> Hi Geliang,
>
> The v9 LGTM. Thank you for your help and efforts.
>
> On 2021/7/16 11:04, Geliang Tang wrote:
> > From: Yonglong Li <liyonglong@chinatelecom.cn>
> >
> > 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: mptcp_pm_should_add_addr => 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;
> >
> > v4->v5:
> >  - Patch 3: simple the code of 'mptcp_established_options_add_addr'
> >
> > v5->v6:
> >  - Patch2: fix fails of 'mptcp_join.sh -t'. In mptcp_pm_add_addr_send_ack
> >    without MPTCP_ADD_ADDR_SIGNAL check so pure ack can not be sent for
> >    ADD_ADDR. That cause ADD_ADDR can not be sent in time.
> >  - Patch3: refactor some code according Geliang's suggestions.
> >  - Patch4: modify commit comment
> >
> > v6->v7:
> >  - Patch1: use reverse xmas tree order for variables definition
> >  - Patch3: refactor some code according Geliang's suggestions.
> >  - add a Patch4: remove some double-check
> >
> > v7->v8:
> >  - Patch1,2: exchange patch1 and patch2
> >  - Patch3: refactor some code according Geliang's suggestions.
> >  - remove patch "remove some double check", Geliang think it's unnecessary
> >
> > v8->v9:
> >  - Keep mptcp_add_addr_len unchanged.
> >  - populate opts->local or opts->remote after the length check, don't
> >    populate both of them.
> >  - add back 'echo' and 'port' arguments of mptcp_pm_add_addr_signal().
> >  - add a new arguments drop_other_suboptions for
> >    mptcp_pm_add_addr_signal(), and do the drop_other_suboptions check in it.
> >  - drop other suboptions in mptcp_established_options_add_addr() after the
> >    length check.
> >  - split the drop_other_suboptions code into a new patch.
> >  - add a new selftest case.
> >
> > Yonglong Li (6):
> >   mptcp: move drop_other_suboptions check under pm lock
> >   mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
> >   mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
> >   mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
> >   mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
> >   selftests: mptcp: add_addr and echo race test
> >
> >  include/net/mptcp.h                           |  3 +-
> >  net/mptcp/options.c                           | 54 +++++++++---------
> >  net/mptcp/pm.c                                | 57 +++++++++++++------
> >  net/mptcp/pm_netlink.c                        | 10 ++--
> >  net/mptcp/protocol.h                          | 24 ++++----
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++
> >  6 files changed, 97 insertions(+), 66 deletions(-)
> >
>
> --
> Li YongLong

Re: [MPTCP][PATCH v9 mptcp-next 0/6] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[Yonglong Li]

Hi Geliang,

I tested v9 and don't get any issue last weekend and today.
I think v9 is ok.

On 2021/7/16 18:13, Geliang Tang wrote:
> Thanks, Yonglong,
> 
> I'll do more tests for v9. And please test v9 on your computer too.
> 
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月16日周五 下午5:36写道：
>>
>>
>> Hi Geliang,
>>
>> The v9 LGTM. Thank you for your help and efforts.
>>
>> On 2021/7/16 11:04, Geliang Tang wrote:
>>> From: Yonglong Li <liyonglong@chinatelecom.cn>
>>>
>>> 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: mptcp_pm_should_add_addr => 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;
>>>
>>> v4->v5:
>>>  - Patch 3: simple the code of 'mptcp_established_options_add_addr'
>>>
>>> v5->v6:
>>>  - Patch2: fix fails of 'mptcp_join.sh -t'. In mptcp_pm_add_addr_send_ack
>>>    without MPTCP_ADD_ADDR_SIGNAL check so pure ack can not be sent for
>>>    ADD_ADDR. That cause ADD_ADDR can not be sent in time.
>>>  - Patch3: refactor some code according Geliang's suggestions.
>>>  - Patch4: modify commit comment
>>>
>>> v6->v7:
>>>  - Patch1: use reverse xmas tree order for variables definition
>>>  - Patch3: refactor some code according Geliang's suggestions.
>>>  - add a Patch4: remove some double-check
>>>
>>> v7->v8:
>>>  - Patch1,2: exchange patch1 and patch2
>>>  - Patch3: refactor some code according Geliang's suggestions.
>>>  - remove patch "remove some double check", Geliang think it's unnecessary
>>>
>>> v8->v9:
>>>  - Keep mptcp_add_addr_len unchanged.
>>>  - populate opts->local or opts->remote after the length check, don't
>>>    populate both of them.
>>>  - add back 'echo' and 'port' arguments of mptcp_pm_add_addr_signal().
>>>  - add a new arguments drop_other_suboptions for
>>>    mptcp_pm_add_addr_signal(), and do the drop_other_suboptions check in it.
>>>  - drop other suboptions in mptcp_established_options_add_addr() after the
>>>    length check.
>>>  - split the drop_other_suboptions code into a new patch.
>>>  - add a new selftest case.
>>>
>>> Yonglong Li (6):
>>>   mptcp: move drop_other_suboptions check under pm lock
>>>   mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
>>>   mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
>>>   mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
>>>   mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
>>>   selftests: mptcp: add_addr and echo race test
>>>
>>>  include/net/mptcp.h                           |  3 +-
>>>  net/mptcp/options.c                           | 54 +++++++++---------
>>>  net/mptcp/pm.c                                | 57 +++++++++++++------
>>>  net/mptcp/pm_netlink.c                        | 10 ++--
>>>  net/mptcp/protocol.h                          | 24 ++++----
>>>  .../testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++
>>>  6 files changed, 97 insertions(+), 66 deletions(-)
>>>
>>
>> --
>> Li YongLong
> 

-- 
Li YongLong

Re: [MPTCP][PATCH v9 mptcp-next 0/6] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[Geliang Tang]

Ok, thanks.

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月19日周一 下午5:57写道：
>
> Hi Geliang,
>
> I tested v9 and don't get any issue last weekend and today.
> I think v9 is ok.
>
> On 2021/7/16 18:13, Geliang Tang wrote:
> > Thanks, Yonglong,
> >
> > I'll do more tests for v9. And please test v9 on your computer too.
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月16日周五 下午5:36写道：
> >>
> >>
> >> Hi Geliang,
> >>
> >> The v9 LGTM. Thank you for your help and efforts.
> >>
> >> On 2021/7/16 11:04, Geliang Tang wrote:
> >>> From: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>
> >>> 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: mptcp_pm_should_add_addr => 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;
> >>>
> >>> v4->v5:
> >>>  - Patch 3: simple the code of 'mptcp_established_options_add_addr'
> >>>
> >>> v5->v6:
> >>>  - Patch2: fix fails of 'mptcp_join.sh -t'. In mptcp_pm_add_addr_send_ack
> >>>    without MPTCP_ADD_ADDR_SIGNAL check so pure ack can not be sent for
> >>>    ADD_ADDR. That cause ADD_ADDR can not be sent in time.
> >>>  - Patch3: refactor some code according Geliang's suggestions.
> >>>  - Patch4: modify commit comment
> >>>
> >>> v6->v7:
> >>>  - Patch1: use reverse xmas tree order for variables definition
> >>>  - Patch3: refactor some code according Geliang's suggestions.
> >>>  - add a Patch4: remove some double-check
> >>>
> >>> v7->v8:
> >>>  - Patch1,2: exchange patch1 and patch2
> >>>  - Patch3: refactor some code according Geliang's suggestions.
> >>>  - remove patch "remove some double check", Geliang think it's unnecessary
> >>>
> >>> v8->v9:
> >>>  - Keep mptcp_add_addr_len unchanged.
> >>>  - populate opts->local or opts->remote after the length check, don't
> >>>    populate both of them.
> >>>  - add back 'echo' and 'port' arguments of mptcp_pm_add_addr_signal().
> >>>  - add a new arguments drop_other_suboptions for
> >>>    mptcp_pm_add_addr_signal(), and do the drop_other_suboptions check in it.
> >>>  - drop other suboptions in mptcp_established_options_add_addr() after the
> >>>    length check.
> >>>  - split the drop_other_suboptions code into a new patch.
> >>>  - add a new selftest case.
> >>>
> >>> Yonglong Li (6):
> >>>   mptcp: move drop_other_suboptions check under pm lock
> >>>   mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
> >>>   mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
> >>>   mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
> >>>   mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
> >>>   selftests: mptcp: add_addr and echo race test
> >>>
> >>>  include/net/mptcp.h                           |  3 +-
> >>>  net/mptcp/options.c                           | 54 +++++++++---------
> >>>  net/mptcp/pm.c                                | 57 +++++++++++++------
> >>>  net/mptcp/pm_netlink.c                        | 10 ++--
> >>>  net/mptcp/protocol.h                          | 24 ++++----
> >>>  .../testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++
> >>>  6 files changed, 97 insertions(+), 66 deletions(-)
> >>>
> >>
> >> --
> >> Li YongLong
> >
>
> --
> Li YongLong

Re: [MPTCP][PATCH v9 mptcp-next 0/6] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[Matthieu Baerts]

Hi Geliang, Yonglong,

On 16/07/2021 05:12, Geliang Tang wrote:
> From: Yonglong Li <liyonglong@chinatelecom.cn>
> 
> 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.

Thank you for these patches!

I just applied them in our tree to get more feedback.

- fea9f8b0171f: mptcp: move drop_other_suboptions check under pm lock
- 097eb5756742: mptcp: make MPTCP_ADD_ADDR_SIGNAL and
MPTCP_ADD_ADDR_ECHO separate
- edaad5420e34: mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal
each other
- b140ddc3f7eb: mptcp: build ADD_ADDR/echo-ADD_ADDR option according
pm.add_signal
- 90f451056fe9: mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
- bb8637ec2243: selftests: mptcp: add_addr and echo race test
- Results: 519fb8907fda..3a1215522891

We can leave them a bit in our tree for more tests and also to wait for
Mat's feedback as he helped reviewing the previous versions. No urgency
there anyway.

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210722T091935
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210722T091935

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net