[MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other"

[MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other"

[Geliang Tang]

This line 'if ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)))' should
be:
        if (msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO))

Since I'll keep the echo argument in mptcp_pm_add_addr_signal(), just use
*echo here.

Like the 'rm_addr' variable in mptcp_pm_rm_addr_signal(), use a new
variable 'add_addr' in mptcp_pm_add_addr_signal() too.

v2:
 - drop READ_ONCE()
 - drop mptcp_pm_should_add_signal_echo()
 - use & instead of &=

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/pm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index c9622696716e..792940dbe662 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -257,6 +257,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			      struct mptcp_addr_info *saddr, bool *echo, bool *port)
 {
 	int ret = false;
+	u8 add_addr;
 
 	spin_lock_bh(&msk->pm.lock);
 
@@ -271,10 +272,11 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 		goto out_unlock;
 
 	*saddr = msk->pm.local;
-	if ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)))
-		WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO));
+	if (*echo)
+		add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
 	else
-		WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
+		add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
+	WRITE_ONCE(msk->pm.addr_signal, add_addr);
 	ret = true;
 
 out_unlock:
-- 
2.31.1

[MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"

[Geliang Tang]

In v8, opts->local and opts->remote in mptcp_pm_add_addr_signal need be
populated after the length check, not before the check.

This patch fixed it. And keep the more original code unchanged, just do
the least, necessary modifications.

- 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.
- update mptcp_pm_should_add_signal_ipv6() and
  mptcp_pm_should_add_signal_port(), not drop them. They will be used in
  the drop_other_suboptions check and in mptcp_pm_nl_addr_send_ack() in the
  next squash-to patch.

v2:
 - move the drop_other_suboptions check into mptcp_pm_add_addr_signal().
 - drop other suboptions in mptcp_established_options_add_addr() after the
   length check.
 - add back mptcp_pm_should_add_signal_ipv6() and
   mptcp_pm_should_add_signal_port().
 - populate opts->local or opts->remote, not both of them.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/options.c  | 43 +++++++++++++++++++------------------------
 net/mptcp/pm.c       | 29 +++++++++++++++++------------
 net/mptcp/protocol.h | 30 ++++++++++++------------------
 3 files changed, 48 insertions(+), 54 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 5c0ad9b90866..37ff15aeb2f7 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -663,42 +663,40 @@ 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;
-	u8 add_addr;
+	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, &add_addr))
+	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts,
+		    &echo, &port, &drop_other_suboptions))
 		return false;
 
-	if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
-	     ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
-	      (opts->local.family == AF_INET6 || opts->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;
+	if (drop_other_suboptions)
 		remaining += opt_size;
-		drop_other_suboptions = true;
-	}
-
-	len = mptcp_add_addr_len(opts, add_addr);
+	family = echo ? opts->remote.family : opts->local.family;
+	len = mptcp_add_addr_len(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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
-	    (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
+	if (!echo) {
 		opts->ahmac = add_addr_generate_hmac(msk->local_key,
 						     msk->remote_key,
 						     &opts->local);
 	}
-	pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
-		 add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
-		 ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.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;
 }
@@ -1253,13 +1251,10 @@ 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->remote;
+		struct mptcp_addr_info *addr = opts->ahmac ? &opts->local : &opts->remote;
 		u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
 		u8 echo = MPTCP_ADDR_ECHO;
 
-		if (opts->ahmac)
-			addr = &opts->local;
-
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 		if (addr->family == AF_INET6)
 			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 88b5db9114f4..62734d6b534d 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -255,10 +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_out_options *opts,  u8 *add_addr)
+			      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);
 
@@ -266,25 +268,28 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 	if (!mptcp_pm_should_add_signal(msk))
 		goto out_unlock;
 
-	opts->local = msk->pm.local;
-	opts->remote = msk->pm.remote;
-	*add_addr = msk->pm.addr_signal;
-
-	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))) &&
+	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;
 	}
 
-	if (remaining < mptcp_add_addr_len(opts, *add_addr))
+	*echo = mptcp_pm_should_add_signal_echo(msk);
+	*port = mptcp_pm_should_add_signal_port(msk);
+
+	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 937e0309e340..08a76eaea2e5 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -753,12 +753,14 @@ static inline bool mptcp_pm_should_add_signal_echo(struct mptcp_sock *msk)
 
 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);
+	return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.family == AF_INET6) ||
+		(mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.family == AF_INET6);
 }
 
 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);
+	return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.port) ||
+		(mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.port);
 }
 
 static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
@@ -766,25 +768,16 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
 	return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
 }
 
-static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts,
-					      u8 add_addr)
+static inline unsigned int mptcp_add_addr_len(u8 family, bool echo, bool port)
 {
-	struct mptcp_addr_info *addr = &opts->remote;
-	u8 len = 0;
+	u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
 
-	if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
-	    (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
-		addr = &opts->local;
+	if (family == AF_INET6)
+		len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+	if (!echo)
 		len += MPTCPOPT_THMAC_LEN;
-	}
-
-	if (addr->family == AF_INET6)
-		len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
-	else
-		len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
-
 	/* account for 2 trailing 'nop' options */
-	if (addr->port)
+	if (port)
 		len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
 
 	return len;
@@ -800,7 +793,8 @@ 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_out_options *opts,  u8 *add_addr);
+			      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);
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
-- 
2.31.1

Re: [MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"

[Geliang Tang]

This squash-to patch will conflict with "mptcp: remove
MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT":

Auto-merging net/mptcp/protocol.h
CONFLICT (content): Merge conflict in net/mptcp/protocol.h
Auto-merging net/mptcp/pm.c
error: could not apply bf1fec79a2bf... mptcp: remove
MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT

Please fix it like this:

 <<<<<<< HEAD
 static inline bool mptcp_pm_should_add_signal_ipv6(struct mptcp_sock *msk)
 {
         return (mptcp_pm_should_add_signal_addr(msk) &&
msk->pm.local.family == AF_INET6) ||
                 (mptcp_pm_should_add_signal_echo(msk) &&
msk->pm.remote.family == AF_INET6);
 }

 static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk)
 {
         return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.port) ||
                 (mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.port);
 }

 =======
 >>>>>>> bf1fec79a2bf (mptcp: remove MPTCP_ADD_ADDR_IPV6 and
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);
 }

->

 static inline bool mptcp_pm_should_add_signal_ipv6(struct mptcp_sock *msk)
 {
         return (mptcp_pm_should_add_signal_addr(msk) &&
msk->pm.local.family == AF_INET6) ||
                 (mptcp_pm_should_add_signal_echo(msk) &&
msk->pm.remote.family == AF_INET6);
 }

 static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk)
 {
         return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.port) ||
                 (mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.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);
 }

Re: [MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"

[Mat Martineau]

On Tue, 13 Jul 2021, Geliang Tang wrote:

> This squash-to patch will conflict with "mptcp: remove
> MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT":
>
> Auto-merging net/mptcp/protocol.h
> CONFLICT (content): Merge conflict in net/mptcp/protocol.h
> Auto-merging net/mptcp/pm.c
> error: could not apply bf1fec79a2bf... mptcp: remove
> MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
>
> Please fix it like this:
>
> <<<<<<< HEAD
> static inline bool mptcp_pm_should_add_signal_ipv6(struct mptcp_sock *msk)
> {
>         return (mptcp_pm_should_add_signal_addr(msk) &&
> msk->pm.local.family == AF_INET6) ||
>                 (mptcp_pm_should_add_signal_echo(msk) &&
> msk->pm.remote.family == AF_INET6);
> }
>
> static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk)
> {
>         return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.port) ||
>                 (mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.port);
> }
>
> =======
> >>>>>>> bf1fec79a2bf (mptcp: remove MPTCP_ADD_ADDR_IPV6 and
> 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);
> }
>
> ->
>
> static inline bool mptcp_pm_should_add_signal_ipv6(struct mptcp_sock *msk)
> {
>         return (mptcp_pm_should_add_signal_addr(msk) &&
> msk->pm.local.family == AF_INET6) ||
>                 (mptcp_pm_should_add_signal_echo(msk) &&
> msk->pm.remote.family == AF_INET6);
> }
>
> static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk)
> {
>         return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.port) ||
>                 (mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.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);
> }
>
>

Well, it looks like I gave my Reviewed-by tag too soon. I think it would 
be easier to get the correct results by posting more revisions of the 
patch set - don't want to introduce mistakes when trying to resolve these 
conflicts when applying to mptcp_net-next repo!

Geliang and Yonglong, what do you think about posting v9, possibly with 
co-developed-by tags?


--
Mat Martineau
Intel

Re: [MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"

[Geliang Tang]

Hi Mat,

Mat Martineau <mathew.j.martineau@linux.intel.com> 于2021年7月14日周三 上午4:39写道：
>
> On Tue, 13 Jul 2021, Geliang Tang wrote:
>
> > This squash-to patch will conflict with "mptcp: remove
> > MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT":
> >
> > Auto-merging net/mptcp/protocol.h
> > CONFLICT (content): Merge conflict in net/mptcp/protocol.h
> > Auto-merging net/mptcp/pm.c
> > error: could not apply bf1fec79a2bf... mptcp: remove
> > MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
> >
> > Please fix it like this:
> >
> > <<<<<<< HEAD
> > static inline bool mptcp_pm_should_add_signal_ipv6(struct mptcp_sock *msk)
> > {
> >         return (mptcp_pm_should_add_signal_addr(msk) &&
> > msk->pm.local.family == AF_INET6) ||
> >                 (mptcp_pm_should_add_signal_echo(msk) &&
> > msk->pm.remote.family == AF_INET6);
> > }
> >
> > static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk)
> > {
> >         return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.port) ||
> >                 (mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.port);
> > }
> >
> > =======
> > >>>>>>> bf1fec79a2bf (mptcp: remove MPTCP_ADD_ADDR_IPV6 and
> > 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);
> > }
> >
> > ->
> >
> > static inline bool mptcp_pm_should_add_signal_ipv6(struct mptcp_sock *msk)
> > {
> >         return (mptcp_pm_should_add_signal_addr(msk) &&
> > msk->pm.local.family == AF_INET6) ||
> >                 (mptcp_pm_should_add_signal_echo(msk) &&
> > msk->pm.remote.family == AF_INET6);
> > }
> >
> > static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk)
> > {
> >         return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.port) ||
> >                 (mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.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);
> > }
> >
> >
>
> Well, it looks like I gave my Reviewed-by tag too soon. I think it would
> be easier to get the correct results by posting more revisions of the
> patch set - don't want to introduce mistakes when trying to resolve these
> conflicts when applying to mptcp_net-next repo!
>
> Geliang and Yonglong, what do you think about posting v9, possibly with
> co-developed-by tags?
>

Sounds good. I'll send a v9, with my co-developed-by tags, and keep Yonglong
as the author, when the code is ready and the test passes.

Thanks,
-Geliang

>
> --
> Mat Martineau
> Intel

Re: [MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"

[Yonglong Li]

On 2021/7/13 14:44, Geliang Tang wrote:
> In v8, opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> populated after the length check, not before the check.
> 
> This patch fixed it. And keep the more original code unchanged, just do
> the least, necessary modifications.
> 
> - 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.
> - update mptcp_pm_should_add_signal_ipv6() and
>   mptcp_pm_should_add_signal_port(), not drop them. They will be used in
>   the drop_other_suboptions check and in mptcp_pm_nl_addr_send_ack() in the
>   next squash-to patch.
> 
> v2:
>  - move the drop_other_suboptions check into mptcp_pm_add_addr_signal().
>  - drop other suboptions in mptcp_established_options_add_addr() after the
>    length check.
>  - add back mptcp_pm_should_add_signal_ipv6() and
>    mptcp_pm_should_add_signal_port().
>  - populate opts->local or opts->remote, not both of them.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/mptcp/options.c  | 43 +++++++++++++++++++------------------------
>  net/mptcp/pm.c       | 29 +++++++++++++++++------------
>  net/mptcp/protocol.h | 30 ++++++++++++------------------
>  3 files changed, 48 insertions(+), 54 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 5c0ad9b90866..37ff15aeb2f7 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -663,42 +663,40 @@ 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;
> -	u8 add_addr;
> +	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, &add_addr))
> +	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts,
> +		    &echo, &port, &drop_other_suboptions))
>  		return false;
>  
> -	if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> -	     ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> -	      (opts->local.family == AF_INET6 || opts->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;
> +	if (drop_other_suboptions)
>  		remaining += opt_size;
> -		drop_other_suboptions = true;
> -	}
> -
> -	len = mptcp_add_addr_len(opts, add_addr);
> +	family = echo ? opts->remote.family : opts->local.family;
> +	len = mptcp_add_addr_len(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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> -	    (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> +	if (!echo) {
>  		opts->ahmac = add_addr_generate_hmac(msk->local_key,
>  						     msk->remote_key,
>  						     &opts->local);
>  	}
> -	pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> -		 add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
> -		 ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.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;
>  }
> @@ -1253,13 +1251,10 @@ 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->remote;
> +		struct mptcp_addr_info *addr = opts->ahmac ? &opts->local : &opts->remote;
>  		u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>  		u8 echo = MPTCP_ADDR_ECHO;
>  
> -		if (opts->ahmac)
> -			addr = &opts->local;
> -
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>  		if (addr->family == AF_INET6)
>  			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 88b5db9114f4..62734d6b534d 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -255,10 +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_out_options *opts,  u8 *add_addr)
> +			      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);
>  
> @@ -266,25 +268,28 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>  	if (!mptcp_pm_should_add_signal(msk))
>  		goto out_unlock;
>  
> -	opts->local = msk->pm.local;
> -	opts->remote = msk->pm.remote;
> -	*add_addr = msk->pm.addr_signal;
> -
> -	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))) &&
> +	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;
>  	}
>  
> -	if (remaining < mptcp_add_addr_len(opts, *add_addr))
> +	*echo = mptcp_pm_should_add_signal_echo(msk);
> +	*port = mptcp_pm_should_add_signal_port(msk);
> +
> +	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 937e0309e340..08a76eaea2e5 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -753,12 +753,14 @@ static inline bool mptcp_pm_should_add_signal_echo(struct mptcp_sock *msk)
>  
>  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);
> +	return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.family == AF_INET6) ||
> +		(mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.family == AF_INET6);
>  }Should we use READ_ONCE to read remote.family?

>  
>  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);
> +	return (mptcp_pm_should_add_signal_addr(msk) && msk->pm.local.port) ||
> +		(mptcp_pm_should_add_signal_echo(msk) && msk->pm.remote.port);
>  }
Should we use READ_ONCE to read remote.port?

>  
>  static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> @@ -766,25 +768,16 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>  	return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>  }
>  
> -static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts,
> -					      u8 add_addr)
> +static inline unsigned int mptcp_add_addr_len(u8 family, bool echo, bool port)
>  {
> -	struct mptcp_addr_info *addr = &opts->remote;
> -	u8 len = 0;
> +	u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>  
> -	if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> -	    (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> -		addr = &opts->local;
> +	if (family == AF_INET6)
> +		len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +	if (!echo)
>  		len += MPTCPOPT_THMAC_LEN;
> -	}
> -
> -	if (addr->family == AF_INET6)
> -		len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> -	else
> -		len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> -
>  	/* account for 2 trailing 'nop' options */
> -	if (addr->port)
> +	if (port)
>  		len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>  
>  	return len;
> @@ -800,7 +793,8 @@ 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_out_options *opts,  u8 *add_addr);
> +			      struct mptcp_out_options *opts, bool *echo,
> +			      bool *port, bool *drop_other_suboptions);
Uhm... I think this function has too many parameters... but not serious.

>  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);
> 

-- 
Li YongLong

[MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT"

[Geliang Tang]

Keep the debug info for "send ack".

Don't drop mptcp_pm_should_add_signal_ipv6() and
mptcp_pm_should_add_signal_port().

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/pm_netlink.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 3e35720317ae..2cd6caaedb08 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -543,8 +543,11 @@ 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",
-			 mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
+		pr_debug("send ack for %s%s%s",
+			 mptcp_pm_should_add_signal_addr(msk) ? "add_addr" :
+			 (mptcp_pm_should_add_signal_echo(msk) ? "add_echo" : "rm_addr"),
+			 mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
+			 mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
 
 		slow = lock_sock_fast(ssk);
 		tcp_send_ack(ssk);
-- 
2.31.1

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT"

[Yonglong Li]

On 2021/7/13 14:44, Geliang Tang wrote:
> Keep the debug info for "send ack".
> 
> Don't drop mptcp_pm_should_add_signal_ipv6() and
> mptcp_pm_should_add_signal_port().
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/mptcp/pm_netlink.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 3e35720317ae..2cd6caaedb08 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -543,8 +543,11 @@ 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",
> -			 mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
> +		pr_debug("send ack for %s%s%s",
> +			 mptcp_pm_should_add_signal_addr(msk) ? "add_addr" :
> +			 (mptcp_pm_should_add_signal_echo(msk) ? "add_echo" : "rm_addr"),
> +			 mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
> +			 mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
>  
>  		slow = lock_sock_fast(ssk);
>  		tcp_send_ack(ssk);
> 
Hi Geliang,

I think the debug log will be incorrect if add_addr and add_echo events trigger at the same time.
WDYT?

-- 
Li YongLong

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT"

[Geliang Tang]

Hi Yonglong,

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月13日周二 下午6:30写道：
>
>
>
> On 2021/7/13 14:44, Geliang Tang wrote:
> > Keep the debug info for "send ack".
> >
> > Don't drop mptcp_pm_should_add_signal_ipv6() and
> > mptcp_pm_should_add_signal_port().
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> >  net/mptcp/pm_netlink.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 3e35720317ae..2cd6caaedb08 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -543,8 +543,11 @@ 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",
> > -                      mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
> > +             pr_debug("send ack for %s%s%s",
> > +                      mptcp_pm_should_add_signal_addr(msk) ? "add_addr" :
> > +                      (mptcp_pm_should_add_signal_echo(msk) ? "add_echo" : "rm_addr"),
> > +                      mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
> > +                      mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
> >
> >               slow = lock_sock_fast(ssk);
> >               tcp_send_ack(ssk);
> >
> Hi Geliang,
>
> I think the debug log will be incorrect if add_addr and add_echo events trigger at the same time.
> WDYT?

Yes, how about moving this pr_debug line under the pm lock, just swap this
pr_debug line with the spin_unlock_bh line?

If so, no need to use READ_ONCE() in mptcp_pm_should_add_signal_ipv6/port
too.


I had tested this squash-to patches all night yesterday. And I got this
error in the debug log:

add_signal error, add_addr=2, echo=1

This means the race occurs, right?

Does it mean that this version cannot deal with the race either? I'm not
sure.

Could you please share your test scripts to me?

Thanks,
-Geliang



>
> --
> Li YongLong

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT"

[Yonglong Li]

Hi Geliang,

On 2021/7/14 11:10, Geliang Tang wrote:
> Hi Yonglong,
> 
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月13日周二 下午6:30写道：
>>
>>
>>
>> On 2021/7/13 14:44, Geliang Tang wrote:
>>> Keep the debug info for "send ack".
>>>
>>> Don't drop mptcp_pm_should_add_signal_ipv6() and
>>> mptcp_pm_should_add_signal_port().
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>>  net/mptcp/pm_netlink.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 3e35720317ae..2cd6caaedb08 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -543,8 +543,11 @@ 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",
>>> -                      mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
>>> +             pr_debug("send ack for %s%s%s",
>>> +                      mptcp_pm_should_add_signal_addr(msk) ? "add_addr" :
>>> +                      (mptcp_pm_should_add_signal_echo(msk) ? "add_echo" : "rm_addr"),
>>> +                      mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
>>> +                      mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
>>>
>>>               slow = lock_sock_fast(ssk);
>>>               tcp_send_ack(ssk);
>>>
>> Hi Geliang,
>>
>> I think the debug log will be incorrect if add_addr and add_echo events trigger at the same time.
>> WDYT?
> 
> Yes, how about moving this pr_debug line under the pm lock, just swap this
> pr_debug line with the spin_unlock_bh line?
> 
> If so, no need to use READ_ONCE() in mptcp_pm_should_add_signal_ipv6/port
> too.
I prefer to remove ipv6 check just like v8. if you want to debug we can get more detail info
from debug log in mptcp_established_options_add_addr.

> 
> 
> I had tested this squash-to patches all night yesterday. And I got this
> error in the debug log:
> 
> add_signal error, add_addr=2, echo=1
> 
> This means the race occurs, right?
> 
It seams like anther race case or bug? if more than one 'echo add addr' event be trigger at the same
time this log will be show.

> Does it mean that this version cannot deal with the race either? I'm not
> sure.
> 
> Could you please share your test scripts to me?
> 
I add a test case in mptcp_join.sh, and run 'mptcp_join.sh -X -c' in a loop, if test case failed the race maybe occurs.
and then analsys the pcap file to check the race.

the test case add in mptcp_join.sh:

diff --git a/mptcp_join.sh b/mptcp_join.sh
index 523c779..162a451 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1462,6 +1462,24 @@ checksum_tests()
        chk_csum_nr "checksum test 1 0"
 }

+xxoo()
+{
+       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
+}
+
+
 all_tests()
 {
        subflows_tests
@@ -1566,6 +1584,9 @@ while getopts 'fsltra64bpkchCS' opt; do
                S)
                        checksum_tests
                        ;;
+               X)
+                       xxoo
+                       ;;
                c)
                        ;;
                C)


> Thanks,
> -Geliang
> 
> 
> 
>>
>> --
>> Li YongLong
> 
> 

-- 
Li YongLong

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT"

[Geliang Tang]

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月14日周三 下午5:49写道：
>
> Hi Geliang,
>
> On 2021/7/14 11:10, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月13日周二 下午6:30写道：
> >>
> >>
> >>
> >> On 2021/7/13 14:44, Geliang Tang wrote:
> >>> Keep the debug info for "send ack".
> >>>
> >>> Don't drop mptcp_pm_should_add_signal_ipv6() and
> >>> mptcp_pm_should_add_signal_port().
> >>>
> >>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> >>> ---
> >>>  net/mptcp/pm_netlink.c | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> >>> index 3e35720317ae..2cd6caaedb08 100644
> >>> --- a/net/mptcp/pm_netlink.c
> >>> +++ b/net/mptcp/pm_netlink.c
> >>> @@ -543,8 +543,11 @@ 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",
> >>> -                      mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
> >>> +             pr_debug("send ack for %s%s%s",
> >>> +                      mptcp_pm_should_add_signal_addr(msk) ? "add_addr" :
> >>> +                      (mptcp_pm_should_add_signal_echo(msk) ? "add_echo" : "rm_addr"),
> >>> +                      mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
> >>> +                      mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
> >>>
> >>>               slow = lock_sock_fast(ssk);
> >>>               tcp_send_ack(ssk);
> >>>
> >> Hi Geliang,
> >>
> >> I think the debug log will be incorrect if add_addr and add_echo events trigger at the same time.
> >> WDYT?
> >
> > Yes, how about moving this pr_debug line under the pm lock, just swap this
> > pr_debug line with the spin_unlock_bh line?
> >
> > If so, no need to use READ_ONCE() in mptcp_pm_should_add_signal_ipv6/port
> > too.
> I prefer to remove ipv6 check just like v8. if you want to debug we can get more detail info
> from debug log in mptcp_established_options_add_addr.

Sure, I had removed mptcp_pm_should_add_signal_ipv6/port in v9.

>
> >
> >
> > I had tested this squash-to patches all night yesterday. And I got this
> > error in the debug log:
> >
> > add_signal error, add_addr=2, echo=1
> >
> > This means the race occurs, right?
> >
> It seams like anther race case or bug? if more than one 'echo add addr' event be trigger at the same
> time this log will be show.

And I had used your testcase to test v9 all night yesterday, and everything
is fine, no this error anymore. It has been tested over 18000 times. So I
think v9 works well. I'll do more test and send out v9 recently.

Do you think 18000 times is enough?  How many times do you usually test
before?

>
> > Does it mean that this version cannot deal with the race either? I'm not
> > sure.
> >
> > Could you please share your test scripts to me?
> >
> I add a test case in mptcp_join.sh, and run 'mptcp_join.sh -X -c' in a loop, if test case failed the race maybe occurs.
> and then analsys the pcap file to check the race.
>
> the test case add in mptcp_join.sh:
>
> diff --git a/mptcp_join.sh b/mptcp_join.sh
> index 523c779..162a451 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1462,6 +1462,24 @@ checksum_tests()
>         chk_csum_nr "checksum test 1 0"
>  }
>
> +xxoo()
> +{
> +       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
> +}

I think this testcase is worth to be added into the selftests script at
the end of the function signal_address_tests() as an extra signal address
test. Do you agree?

If so, I'll add this testcase into v9 and sign your name as the author.

Thanks,
-Geliang




> +
> +
>  all_tests()
>  {
>         subflows_tests
> @@ -1566,6 +1584,9 @@ while getopts 'fsltra64bpkchCS' opt; do
>                 S)
>                         checksum_tests
>                         ;;
> +               X)
> +                       xxoo
> +                       ;;
>                 c)
>                         ;;
>                 C)
>
>
> > Thanks,
> > -Geliang
> >
> >
> >
> >>
> >> --
> >> Li YongLong
> >
> >
>
> --
> Li YongLong
>

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT"

[Yonglong Li]

On 2021/7/15 11:45, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月14日周三 下午5:49写道：
>>
>> Hi Geliang,
>>
>> On 2021/7/14 11:10, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月13日周二 下午6:30写道：
>>>>
>>>>
>>>>
>>>> On 2021/7/13 14:44, Geliang Tang wrote:
>>>>> Keep the debug info for "send ack".
>>>>>
>>>>> Don't drop mptcp_pm_should_add_signal_ipv6() and
>>>>> mptcp_pm_should_add_signal_port().
>>>>>
>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>>>> ---
>>>>>  net/mptcp/pm_netlink.c | 7 +++++--
>>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>>> index 3e35720317ae..2cd6caaedb08 100644
>>>>> --- a/net/mptcp/pm_netlink.c
>>>>> +++ b/net/mptcp/pm_netlink.c
>>>>> @@ -543,8 +543,11 @@ 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",
>>>>> -                      mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
>>>>> +             pr_debug("send ack for %s%s%s",
>>>>> +                      mptcp_pm_should_add_signal_addr(msk) ? "add_addr" :
>>>>> +                      (mptcp_pm_should_add_signal_echo(msk) ? "add_echo" : "rm_addr"),
>>>>> +                      mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
>>>>> +                      mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
>>>>>
>>>>>               slow = lock_sock_fast(ssk);
>>>>>               tcp_send_ack(ssk);
>>>>>
>>>> Hi Geliang,
>>>>
>>>> I think the debug log will be incorrect if add_addr and add_echo events trigger at the same time.
>>>> WDYT?
>>>
>>> Yes, how about moving this pr_debug line under the pm lock, just swap this
>>> pr_debug line with the spin_unlock_bh line?
>>>
>>> If so, no need to use READ_ONCE() in mptcp_pm_should_add_signal_ipv6/port
>>> too.
>> I prefer to remove ipv6 check just like v8. if you want to debug we can get more detail info
>> from debug log in mptcp_established_options_add_addr.
> 
> Sure, I had removed mptcp_pm_should_add_signal_ipv6/port in v9.
> 
>>
>>>
>>>
>>> I had tested this squash-to patches all night yesterday. And I got this
>>> error in the debug log:
>>>
>>> add_signal error, add_addr=2, echo=1
>>>
>>> This means the race occurs, right?
>>>
>> It seams like anther race case or bug? if more than one 'echo add addr' event be trigger at the same
>> time this log will be show.
> 
> And I had used your testcase to test v9 all night yesterday, and everything
> is fine, no this error anymore. It has been tested over 18000 times. So I
> think v9 works well. I'll do more test and send out v9 recently.
> 
> Do you think 18000 times is enough?  How many times do you usually test
> before?
> 
I think 18000 times is enough. Thanks for your patience.

>>
>>> Does it mean that this version cannot deal with the race either? I'm not
>>> sure.
>>>
>>> Could you please share your test scripts to me?
>>>
>> I add a test case in mptcp_join.sh, and run 'mptcp_join.sh -X -c' in a loop, if test case failed the race maybe occurs.
>> and then analsys the pcap file to check the race.
>>
>> the test case add in mptcp_join.sh:
>>
>> diff --git a/mptcp_join.sh b/mptcp_join.sh
>> index 523c779..162a451 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1462,6 +1462,24 @@ checksum_tests()
>>         chk_csum_nr "checksum test 1 0"
>>  }
>>
>> +xxoo()
>> +{
>> +       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
>> +}
> 
> I think this testcase is worth to be added into the selftests script at
> the end of the function signal_address_tests() as an extra signal address
> test. Do you agree?
> 
> If so, I'll add this testcase into v9 and sign your name as the author.
> 
> Thanks,
> -Geliang
> 
> 
Agree. Thank you again.

> 
> 
>> +
>> +
>>  all_tests()
>>  {
>>         subflows_tests
>> @@ -1566,6 +1584,9 @@ while getopts 'fsltra64bpkchCS' opt; do
>>                 S)
>>                         checksum_tests
>>                         ;;
>> +               X)
>> +                       xxoo
>> +                       ;;
>>                 c)
>>                         ;;
>>                 C)
>>
>>
>>> Thanks,
>>> -Geliang
>>>
>>>
>>>
>>>>
>>>> --
>>>> Li YongLong
>>>
>>>
>>
>> --
>> Li YongLong
>>
> 

-- 
Li YongLong

Re: [MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other"

[Geliang Tang]

This squash-to patch will conflict with "mptcp: build
ADD_ADDR/echo-ADD_ADDR option according pm.add_signal":

Auto-merging net/mptcp/pm.c
CONFLICT (content): Merge conflict in net/mptcp/pm.c
error: could not apply 205974fb3505... mptcp: build
ADD_ADDR/echo-ADD_ADDR option according pm.add_signal

Please fix it like this:

 <<<<<<< HEAD
         *saddr = msk->pm.local;
         if (*echo)
                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
 =======
         if ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)))
                 WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal &
~BIT(MPTCP_ADD_ADDR_ECHO));
 >>>>>>> 205974fb3505 (mptcp: build ADD_ADDR/echo-ADD_ADDR option
according pm.add_signal)
         else
                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);

->

         *saddr = msk->pm.local;
         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);