[MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate"

[MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate"

[Geliang Tang]

A small cleanup.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 30deb76fa5d0..1eeecd68f159 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -18,7 +18,7 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 {
 	u8 add_addr = READ_ONCE(msk->pm.addr_signal);
 
-	pr_debug("msk=%p, local_id=%d, echo:%d", msk, addr->id, echo);
+	pr_debug("msk=%p, local_id=%d, echo=%d", msk, addr->id, echo);
 
 	lockdep_assert_held(&msk->pm.lock);
 
-- 
2.31.1

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

[Geliang Tang]

Add READ_ONCE() for reading msk->pm.addr_signal.

Use mptcp_pm_should_add_signal_echo instead of open coding.

Use '&=' to clear flag.

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index c9622696716e..be16da2dcb6b 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,12 @@ 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));
+	add_addr = READ_ONCE(msk->pm.addr_signal);
+	if (mptcp_pm_should_add_signal_echo(msk))
+		add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
 	else
-		WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
+		add_addr &= ~BIT(MPTCP_ADD_ADDR_SIGNAL);
+	WRITE_ONCE(msk->pm.addr_signal, add_addr);
 	ret = true;
 
 out_unlock:
@@ -294,7 +297,8 @@ 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);
+	rm_addr = READ_ONCE(msk->pm.addr_signal);
+	rm_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);
 	len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
 	if (len < 0) {
 		WRITE_ONCE(msk->pm.addr_signal, rm_addr);
-- 
2.31.1

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

[Yonglong Li]

On 2021/7/11 23:15, Geliang Tang wrote:
> Add READ_ONCE() for reading msk->pm.addr_signal.
> 
> Use mptcp_pm_should_add_signal_echo instead of open coding.
> 
> Use '&=' to clear flag.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/mptcp/pm.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index c9622696716e..be16da2dcb6b 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,12 @@ 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));
> +	add_addr = READ_ONCE(msk->pm.addr_signal);
> +	if (mptcp_pm_should_add_signal_echo(msk))
> +		add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>  	else
> -		WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
> +		add_addr &= ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> +	WRITE_ONCE(msk->pm.addr_signal, add_addr);
>  	ret = true;
>  
>  out_unlock:
> @@ -294,7 +297,8 @@ 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);
> +	rm_addr = READ_ONCE(msk->pm.addr_signal);
> +	rm_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);
>  	len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
>  	if (len < 0) {
>  		WRITE_ONCE(msk->pm.addr_signal, rm_addr);
> 

These chunk of code is under the pm.lock so It is no need to use READ_ONCE() as Paolo saied before.

-- 
Li YongLong

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

[Geliang Tang]

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:55写道：
>
>
>
> On 2021/7/11 23:15, Geliang Tang wrote:
> > Add READ_ONCE() for reading msk->pm.addr_signal.
> >
> > Use mptcp_pm_should_add_signal_echo instead of open coding.
> >
> > Use '&=' to clear flag.
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> >  net/mptcp/pm.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index c9622696716e..be16da2dcb6b 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,12 @@ 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));
> > +     add_addr = READ_ONCE(msk->pm.addr_signal);
> > +     if (mptcp_pm_should_add_signal_echo(msk))
> > +             add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> >       else
> > -             WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
> > +             add_addr &= ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> > +     WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >       ret = true;
> >
> >  out_unlock:
> > @@ -294,7 +297,8 @@ 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);
> > +     rm_addr = READ_ONCE(msk->pm.addr_signal);
> > +     rm_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);
> >       len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
> >       if (len < 0) {
> >               WRITE_ONCE(msk->pm.addr_signal, rm_addr);
> >
>
> These chunk of code is under the pm.lock so It is no need to use READ_ONCE() as Paolo saied before.

I'll drop this READ_ONCE() in v2.

>
> --
> Li YongLong

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

[Mat Martineau]

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

On Mon, 12 Jul 2021, Geliang Tang wrote:

> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:55写道：
>>
>>
>>
>> On 2021/7/11 23:15, Geliang Tang wrote:
>>> Add READ_ONCE() for reading msk->pm.addr_signal.
>>>
>>> Use mptcp_pm_should_add_signal_echo instead of open coding.
>>>
>>> Use '&=' to clear flag.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>>  net/mptcp/pm.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>> index c9622696716e..be16da2dcb6b 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,12 @@ 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));
>>> +     add_addr = READ_ONCE(msk->pm.addr_signal);

Like below, pm.lock is held here so READ_ONCE() isn't needed.

>>> +     if (mptcp_pm_should_add_signal_echo(msk))
>>> +             add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>       else
>>> -             WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
>>> +             add_addr &= ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>> +     WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>       ret = true;
>>>
>>>  out_unlock:
>>> @@ -294,7 +297,8 @@ 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);
>>> +     rm_addr = READ_ONCE(msk->pm.addr_signal);
>>> +     rm_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);
>>>       len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
>>>       if (len < 0) {
>>>               WRITE_ONCE(msk->pm.addr_signal, rm_addr);
>>>
>>
>> These chunk of code is under the pm.lock so It is no need to use READ_ONCE() as Paolo saied before.
>
> I'll drop this READ_ONCE() in v2.
>
>>
>> --
>> Li YongLong
>
>

--
Mat Martineau
Intel

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

[Geliang Tang]

I think there're still some issues in v8:

The remaining value is incorrect since "remaining += opt_size;" in the
"drop other suboptions" checks has been called twice in
mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.

opts->local and opts->remote in mptcp_pm_add_addr_signal need be
populate after the length chech, not before the check.

The squash-to patch keeped the more orignal code unchanged, and just do
the least, necessary modifications.

Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.

Change arguments of mptcp_pm_add_addr_signal.

Keep mptcp_add_addr_len unchanged.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/options.c  | 35 +++++++++++++++++------------------
 net/mptcp/pm.c       | 23 +++++++++--------------
 net/mptcp/protocol.h | 27 +++++++++------------------
 3 files changed, 35 insertions(+), 50 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 5c0ad9b90866..93ad7b134f74 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -663,16 +663,14 @@ 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))
-		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))) &&
+	if ((mptcp_pm_should_add_signal_echo(msk) ||
+	     (mptcp_pm_should_add_signal_addr(msk) &&
+	      (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
 	    skb && skb_is_tcp_pure_ack(skb)) {
 		pr_debug("drop other suboptions");
 		opts->suboptions = 0;
@@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 		drop_other_suboptions = true;
 	}
 
-	len = mptcp_add_addr_len(opts, add_addr);
+	if (!mptcp_pm_should_add_signal(msk) ||
+	    !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
+		return false;
+
+	family = echo ? opts->remote.family : opts->local.family;
+	len = mptcp_add_addr_len(family, echo, port);
 	if (remaining < len)
 		return false;
 
@@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	if (drop_other_suboptions)
 		*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 +1255,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 264f522af530..399b59cb7563 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
 
 /* path manager helpers */
 
-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)
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
+			      struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
+			      bool *echo, bool *port)
 {
 	int ret = false;
 	u8 add_addr;
+	u8 family;
 
 	spin_lock_bh(&msk->pm.lock);
 
@@ -266,21 +267,15 @@ 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;
+	*echo = mptcp_pm_should_add_signal_echo(msk);
+	*port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
 
-	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;
-	}
-
-	if (remaining < mptcp_add_addr_len(opts, *add_addr))
+	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;
+	*daddr = msk->pm.remote;
 	add_addr = READ_ONCE(msk->pm.addr_signal);
 	if (mptcp_pm_should_add_signal_echo(msk))
 		add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 937e0309e340..4b63cc6079fa 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -766,25 +766,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(int 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;
@@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
-			      unsigned int opt_size, unsigned int remaining,
-			      struct mptcp_out_options *opts,  u8 *add_addr);
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
+			      struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
+			      bool *echo, bool *port);
 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 mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"

[Yonglong Li]

On 2021/7/11 23:15, Geliang Tang wrote:
> I think there're still some issues in v8:
> 
> The remaining value is incorrect since "remaining += opt_size;" in the
> "drop other suboptions" checks has been called twice in
> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> 
I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
mptcp_established_options_add_addr.

> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> populate after the length chech, not before the check.]
> 
> The squash-to patch keeped the more orignal code unchanged, and just do
> the least, necessary modifications.
> 
Agree opts->local and opts->remote should be asigned after the length check.
But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
as orignal code, there is a race that:

==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
==> call mptcp_pm_add_addr_signal
==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
==> at this time opts->remote is empty and the length is incorrect.

So I think the orignal code is incorrect. WDYT?

> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> 
> Change arguments of mptcp_pm_add_addr_signal.
> 
> Keep mptcp_add_addr_len unchanged.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
>  net/mptcp/pm.c       | 23 +++++++++--------------
>  net/mptcp/protocol.h | 27 +++++++++------------------
>  3 files changed, 35 insertions(+), 50 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 5c0ad9b90866..93ad7b134f74 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -663,16 +663,14 @@ 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))
> -		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))) &&
> +	if ((mptcp_pm_should_add_signal_echo(msk) ||
> +	     (mptcp_pm_should_add_signal_addr(msk) &&
> +	      (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>  	    skb && skb_is_tcp_pure_ack(skb)) {
>  		pr_debug("drop other suboptions");
>  		opts->suboptions = 0;
> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>  		drop_other_suboptions = true;
>  	}
>  
> -	len = mptcp_add_addr_len(opts, add_addr);
> +	if (!mptcp_pm_should_add_signal(msk) ||
> +	    !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> +		return false;
> +
> +	family = echo ? opts->remote.family : opts->local.family;
> +	len = mptcp_add_addr_len(family, echo, port);
>  	if (remaining < len)
>  		return false;
>  
> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>  	if (drop_other_suboptions)
>  		*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 +1255,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 264f522af530..399b59cb7563 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>  
>  /* path manager helpers */
>  
> -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)
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> +			      struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> +			      bool *echo, bool *port)
>  {
>  	int ret = false;
>  	u8 add_addr;
> +	u8 family;
>  
>  	spin_lock_bh(&msk->pm.lock);
>  
> @@ -266,21 +267,15 @@ 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;
> +	*echo = mptcp_pm_should_add_signal_echo(msk);
> +	*port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>  
> -	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;
> -	}
> -
> -	if (remaining < mptcp_add_addr_len(opts, *add_addr))
> +	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;
> +	*daddr = msk->pm.remote;
>  	add_addr = READ_ONCE(msk->pm.addr_signal);
>  	if (mptcp_pm_should_add_signal_echo(msk))
>  		add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 937e0309e340..4b63cc6079fa 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -766,25 +766,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(int 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;
> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> -			      unsigned int opt_size, unsigned int remaining,
> -			      struct mptcp_out_options *opts,  u8 *add_addr);
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> +			      struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> +			      bool *echo, bool *port);
>  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

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

[Geliang Tang]

Hi Yonglong,

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>
>
>
> On 2021/7/11 23:15, Geliang Tang wrote:
> > I think there're still some issues in v8:
> >
> > The remaining value is incorrect since "remaining += opt_size;" in the
> > "drop other suboptions" checks has been called twice in
> > mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> >
> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> mptcp_established_options_add_addr.
>
> > opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> > populate after the length chech, not before the check.]
> >
> > The squash-to patch keeped the more orignal code unchanged, and just do
> > the least, necessary modifications.
> >
> Agree opts->local and opts->remote should be asigned after the length check.
> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> as orignal code, there is a race that:
>
> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> call mptcp_pm_add_addr_signal
> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> ==> at this time opts->remote is empty and the length is incorrect.
>

What will happen in v8 when this race occurs? How dose v8 deal with the
race?

> So I think the orignal code is incorrect. WDYT?
>
> > Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> >
> > Change arguments of mptcp_pm_add_addr_signal.
> >
> > Keep mptcp_add_addr_len unchanged.
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> >  net/mptcp/options.c  | 35 +++++++++++++++++------------------
> >  net/mptcp/pm.c       | 23 +++++++++--------------
> >  net/mptcp/protocol.h | 27 +++++++++------------------
> >  3 files changed, 35 insertions(+), 50 deletions(-)
> >
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 5c0ad9b90866..93ad7b134f74 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -663,16 +663,14 @@ 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))
> > -             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))) &&
> > +     if ((mptcp_pm_should_add_signal_echo(msk) ||
> > +          (mptcp_pm_should_add_signal_addr(msk) &&
> > +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >           skb && skb_is_tcp_pure_ack(skb)) {
> >               pr_debug("drop other suboptions");
> >               opts->suboptions = 0;
> > @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >               drop_other_suboptions = true;
> >       }
> >
> > -     len = mptcp_add_addr_len(opts, add_addr);
> > +     if (!mptcp_pm_should_add_signal(msk) ||
> > +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> > +             return false;
> > +
> > +     family = echo ? opts->remote.family : opts->local.family;
> > +     len = mptcp_add_addr_len(family, echo, port);
> >       if (remaining < len)
> >               return false;
> >
> > @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >       if (drop_other_suboptions)
> >               *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 +1255,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 264f522af530..399b59cb7563 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >
> >  /* path manager helpers */
> >
> > -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)
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> > +                           bool *echo, bool *port)
> >  {
> >       int ret = false;
> >       u8 add_addr;
> > +     u8 family;
> >
> >       spin_lock_bh(&msk->pm.lock);
> >
> > @@ -266,21 +267,15 @@ 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;
> > +     *echo = mptcp_pm_should_add_signal_echo(msk);
> > +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> >
> > -     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;
> > -     }
> > -
> > -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
> > +     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;
> > +     *daddr = msk->pm.remote;
> >       add_addr = READ_ONCE(msk->pm.addr_signal);
> >       if (mptcp_pm_should_add_signal_echo(msk))
> >               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 937e0309e340..4b63cc6079fa 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -766,25 +766,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(int 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;
> > @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> > -                           unsigned int opt_size, unsigned int remaining,
> > -                           struct mptcp_out_options *opts,  u8 *add_addr);
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> > +                           bool *echo, bool *port);
> >  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

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

[Yonglong Li]

On 2021/7/12 15:33, Geliang Tang wrote:
> Hi Yonglong,
> 
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>>
>>
>>
>> On 2021/7/11 23:15, Geliang Tang wrote:
>>> I think there're still some issues in v8:
>>>
>>> The remaining value is incorrect since "remaining += opt_size;" in the
>>> "drop other suboptions" checks has been called twice in
>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>>>
>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
>> mptcp_established_options_add_addr.
>>
>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
>>> populate after the length chech, not before the check.]
>>>
>>> The squash-to patch keeped the more orignal code unchanged, and just do
>>> the least, necessary modifications.
>>>
>> Agree opts->local and opts->remote should be asigned after the length check.
>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
>> as orignal code, there is a race that:
>>
>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>> ==> call mptcp_pm_add_addr_signal
>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
>> ==> at this time opts->remote is empty and the length is incorrect.
>>
> 
> What will happen in v8 when this race occurs? How dose v8 deal with the
> race?
Hi Geliang, thinks for your patience.

I think v8 doesn't have this issue:
==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
==> use add_addr and opts to check length.
==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.

> 
>> So I think the orignal code is incorrect. WDYT?
>>
>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>>>
>>> Change arguments of mptcp_pm_add_addr_signal.
>>>
>>> Keep mptcp_add_addr_len unchanged.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
>>>  net/mptcp/pm.c       | 23 +++++++++--------------
>>>  net/mptcp/protocol.h | 27 +++++++++------------------
>>>  3 files changed, 35 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index 5c0ad9b90866..93ad7b134f74 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -663,16 +663,14 @@ 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))
>>> -             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))) &&
>>> +     if ((mptcp_pm_should_add_signal_echo(msk) ||
>>> +          (mptcp_pm_should_add_signal_addr(msk) &&
>>> +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>           skb && skb_is_tcp_pure_ack(skb)) {
>>>               pr_debug("drop other suboptions");
>>>               opts->suboptions = 0;
>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>               drop_other_suboptions = true;
>>>       }
>>>
>>> -     len = mptcp_add_addr_len(opts, add_addr);
>>> +     if (!mptcp_pm_should_add_signal(msk) ||
>>> +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
>>> +             return false;
>>> +
>>> +     family = echo ? opts->remote.family : opts->local.family;
>>> +     len = mptcp_add_addr_len(family, echo, port);
>>>       if (remaining < len)
>>>               return false;
>>>
>>> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>       if (drop_other_suboptions)
>>>               *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 +1255,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 264f522af530..399b59cb7563 100644
>>> --- a/net/mptcp/pm.c
>>> +++ b/net/mptcp/pm.c
>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>
>>>  /* path manager helpers */
>>>
>>> -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)
>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>> +                           bool *echo, bool *port)
>>>  {
>>>       int ret = false;
>>>       u8 add_addr;
>>> +     u8 family;
>>>
>>>       spin_lock_bh(&msk->pm.lock);
>>>
>>> @@ -266,21 +267,15 @@ 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;
>>> +     *echo = mptcp_pm_should_add_signal_echo(msk);
>>> +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>>>
>>> -     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;
>>> -     }
>>> -
>>> -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
>>> +     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;
>>> +     *daddr = msk->pm.remote;
>>>       add_addr = READ_ONCE(msk->pm.addr_signal);
>>>       if (mptcp_pm_should_add_signal_echo(msk))
>>>               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>> index 937e0309e340..4b63cc6079fa 100644
>>> --- a/net/mptcp/protocol.h
>>> +++ b/net/mptcp/protocol.h
>>> @@ -766,25 +766,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(int 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;
>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
>>> -                           unsigned int opt_size, unsigned int remaining,
>>> -                           struct mptcp_out_options *opts,  u8 *add_addr);
>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>> +                           bool *echo, bool *port);
>>>  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
> 

-- 
Li YongLong

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

[Geliang Tang]

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
>
>
>
> On 2021/7/12 15:33, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
> >>
> >>
> >>
> >> On 2021/7/11 23:15, Geliang Tang wrote:
> >>> I think there're still some issues in v8:
> >>>
> >>> The remaining value is incorrect since "remaining += opt_size;" in the
> >>> "drop other suboptions" checks has been called twice in
> >>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> >>>
> >> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> >> mptcp_established_options_add_addr.
> >>
> >>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> >>> populate after the length chech, not before the check.]
> >>>
> >>> The squash-to patch keeped the more orignal code unchanged, and just do
> >>> the least, necessary modifications.
> >>>
> >> Agree opts->local and opts->remote should be asigned after the length check.
> >> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> >> as orignal code, there is a race that:
> >>
> >> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >> ==> call mptcp_pm_add_addr_signal
> >> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> >> ==> at this time opts->remote is empty and the length is incorrect.
> >>
> >
> > What will happen in v8 when this race occurs? How dose v8 deal with the
> > race?
> Hi Geliang, thinks for your patience.
>
> I think v8 doesn't have this issue:
> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
> ==> use add_addr and opts to check length.
> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.

Thanks for your explanation.

I think this squash-to patch did the same thing:

==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
'echo' (echo = false), save the port number to 'port', and save addr
in opts under pm.lock
==> an echo add addr event trigger (pm.addr_signal ==
MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
==> use 'echo' to get the address family, use 'family', 'echo' and
'port' to check length.
==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.

Do you think so?

>
> >
> >> So I think the orignal code is incorrect. WDYT?
> >>
> >>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> >>>
> >>> Change arguments of mptcp_pm_add_addr_signal.
> >>>
> >>> Keep mptcp_add_addr_len unchanged.
> >>>
> >>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> >>> ---
> >>>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
> >>>  net/mptcp/pm.c       | 23 +++++++++--------------
> >>>  net/mptcp/protocol.h | 27 +++++++++------------------
> >>>  3 files changed, 35 insertions(+), 50 deletions(-)
> >>>
> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>> index 5c0ad9b90866..93ad7b134f74 100644
> >>> --- a/net/mptcp/options.c
> >>> +++ b/net/mptcp/options.c
> >>> @@ -663,16 +663,14 @@ 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))
> >>> -             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))) &&
> >>> +     if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>> +          (mptcp_pm_should_add_signal_addr(msk) &&
> >>> +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>>           skb && skb_is_tcp_pure_ack(skb)) {
> >>>               pr_debug("drop other suboptions");
> >>>               opts->suboptions = 0;
> >>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>               drop_other_suboptions = true;
> >>>       }
> >>>
> >>> -     len = mptcp_add_addr_len(opts, add_addr);
> >>> +     if (!mptcp_pm_should_add_signal(msk) ||
> >>> +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> >>> +             return false;
> >>> +
> >>> +     family = echo ? opts->remote.family : opts->local.family;
> >>> +     len = mptcp_add_addr_len(family, echo, port);
> >>>       if (remaining < len)
> >>>               return false;
> >>>
> >>> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>       if (drop_other_suboptions)
> >>>               *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 +1255,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 264f522af530..399b59cb7563 100644
> >>> --- a/net/mptcp/pm.c
> >>> +++ b/net/mptcp/pm.c
> >>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>
> >>>  /* path manager helpers */
> >>>
> >>> -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)
> >>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>> +                           bool *echo, bool *port)
> >>>  {
> >>>       int ret = false;
> >>>       u8 add_addr;
> >>> +     u8 family;
> >>>
> >>>       spin_lock_bh(&msk->pm.lock);
> >>>
> >>> @@ -266,21 +267,15 @@ 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;
> >>> +     *echo = mptcp_pm_should_add_signal_echo(msk);
> >>> +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> >>>
> >>> -     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;
> >>> -     }
> >>> -
> >>> -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
> >>> +     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;
> >>> +     *daddr = msk->pm.remote;
> >>>       add_addr = READ_ONCE(msk->pm.addr_signal);
> >>>       if (mptcp_pm_should_add_signal_echo(msk))
> >>>               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>> index 937e0309e340..4b63cc6079fa 100644
> >>> --- a/net/mptcp/protocol.h
> >>> +++ b/net/mptcp/protocol.h
> >>> @@ -766,25 +766,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(int 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;
> >>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> >>> -                           unsigned int opt_size, unsigned int remaining,
> >>> -                           struct mptcp_out_options *opts,  u8 *add_addr);
> >>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>> +                           bool *echo, bool *port);
> >>>  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
> >
>
> --
> Li YongLong

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

[Geliang Tang]

Geliang Tang <geliangtang@gmail.com> 于2021年7月12日周一 下午4:44写道：
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
> >
> >
> >
> > On 2021/7/12 15:33, Geliang Tang wrote:
> > > Hi Yonglong,
> > >
> > > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
> > >>
> > >>
> > >>
> > >> On 2021/7/11 23:15, Geliang Tang wrote:
> > >>> I think there're still some issues in v8:
> > >>>
> > >>> The remaining value is incorrect since "remaining += opt_size;" in the
> > >>> "drop other suboptions" checks has been called twice in
> > >>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> > >>>
> > >> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> > >> mptcp_established_options_add_addr.
> > >>
> > >>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> > >>> populate after the length chech, not before the check.]
> > >>>
> > >>> The squash-to patch keeped the more orignal code unchanged, and just do
> > >>> the least, necessary modifications.
> > >>>
> > >> Agree opts->local and opts->remote should be asigned after the length check.
> > >> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> > >> as orignal code, there is a race that:
> > >>
> > >> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> > >> ==> call mptcp_pm_add_addr_signal
> > >> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> > >> ==> at this time opts->remote is empty and the length is incorrect.
> > >>
> > >
> > > What will happen in v8 when this race occurs? How dose v8 deal with the
> > > race?
> > Hi Geliang, thinks for your patience.
> >
> > I think v8 doesn't have this issue:
> > ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> > ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
> > ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
> > ==> use add_addr and opts to check length.
> > ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>
> Thanks for your explanation.
>
> I think this squash-to patch did the same thing:
>
> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
> 'echo' (echo = false), save the port number to 'port', and save addr
> in opts under pm.lock
> ==> an echo add addr event trigger (pm.addr_signal ==
> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
> ==> use 'echo' to get the address family, use 'family', 'echo' and
> 'port' to check length.
> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>
> Do you think so?

And one more thing. How do you test this "mptcp: fix conflicts when using
pm.add_signal in ADD_ADDR/echo and RM_ADDR process" series? Do you use our
mptcp_join.sh to test it, or you did some special tests? Does this race
scenario mentioned above easy to reproduce?

I just did the mptcp_join.sh tests for my squash-to patches, and everything
looks fine. If you have some special tests, could you please help me to test
these squash-to patches too? Hope it works in the race scenario.

Thanks.
-Geliang



>
> >
> > >
> > >> So I think the orignal code is incorrect. WDYT?
> > >>
> > >>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> > >>>
> > >>> Change arguments of mptcp_pm_add_addr_signal.
> > >>>
> > >>> Keep mptcp_add_addr_len unchanged.
> > >>>
> > >>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > >>> ---
> > >>>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
> > >>>  net/mptcp/pm.c       | 23 +++++++++--------------
> > >>>  net/mptcp/protocol.h | 27 +++++++++------------------
> > >>>  3 files changed, 35 insertions(+), 50 deletions(-)
> > >>>
> > >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > >>> index 5c0ad9b90866..93ad7b134f74 100644
> > >>> --- a/net/mptcp/options.c
> > >>> +++ b/net/mptcp/options.c
> > >>> @@ -663,16 +663,14 @@ 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))
> > >>> -             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))) &&
> > >>> +     if ((mptcp_pm_should_add_signal_echo(msk) ||
> > >>> +          (mptcp_pm_should_add_signal_addr(msk) &&
> > >>> +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> > >>>           skb && skb_is_tcp_pure_ack(skb)) {
> > >>>               pr_debug("drop other suboptions");
> > >>>               opts->suboptions = 0;
> > >>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > >>>               drop_other_suboptions = true;
> > >>>       }
> > >>>
> > >>> -     len = mptcp_add_addr_len(opts, add_addr);
> > >>> +     if (!mptcp_pm_should_add_signal(msk) ||
> > >>> +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> > >>> +             return false;
> > >>> +
> > >>> +     family = echo ? opts->remote.family : opts->local.family;
> > >>> +     len = mptcp_add_addr_len(family, echo, port);
> > >>>       if (remaining < len)
> > >>>               return false;
> > >>>
> > >>> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > >>>       if (drop_other_suboptions)
> > >>>               *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 +1255,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 264f522af530..399b59cb7563 100644
> > >>> --- a/net/mptcp/pm.c
> > >>> +++ b/net/mptcp/pm.c
> > >>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> > >>>
> > >>>  /* path manager helpers */
> > >>>
> > >>> -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)
> > >>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > >>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> > >>> +                           bool *echo, bool *port)
> > >>>  {
> > >>>       int ret = false;
> > >>>       u8 add_addr;
> > >>> +     u8 family;
> > >>>
> > >>>       spin_lock_bh(&msk->pm.lock);
> > >>>
> > >>> @@ -266,21 +267,15 @@ 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;
> > >>> +     *echo = mptcp_pm_should_add_signal_echo(msk);
> > >>> +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> > >>>
> > >>> -     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;
> > >>> -     }
> > >>> -
> > >>> -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
> > >>> +     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;
> > >>> +     *daddr = msk->pm.remote;
> > >>>       add_addr = READ_ONCE(msk->pm.addr_signal);
> > >>>       if (mptcp_pm_should_add_signal_echo(msk))
> > >>>               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> > >>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > >>> index 937e0309e340..4b63cc6079fa 100644
> > >>> --- a/net/mptcp/protocol.h
> > >>> +++ b/net/mptcp/protocol.h
> > >>> @@ -766,25 +766,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(int 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;
> > >>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> > >>> -                           unsigned int opt_size, unsigned int remaining,
> > >>> -                           struct mptcp_out_options *opts,  u8 *add_addr);
> > >>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > >>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> > >>> +                           bool *echo, bool *port);
> > >>>  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
> > >
> >
> > --
> > Li YongLong

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

[Yonglong Li]

On 2021/7/12 17:07, Geliang Tang wrote:
> Geliang Tang <geliangtang@gmail.com> 于2021年7月12日周一 下午4:44写道：
>>
>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
>>>
>>>
>>>
>>> On 2021/7/12 15:33, Geliang Tang wrote:
>>>> Hi Yonglong,
>>>>
>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>>>>>
>>>>>
>>>>>
>>>>> On 2021/7/11 23:15, Geliang Tang wrote:
>>>>>> I think there're still some issues in v8:
>>>>>>
>>>>>> The remaining value is incorrect since "remaining += opt_size;" in the
>>>>>> "drop other suboptions" checks has been called twice in
>>>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>>>>>>
>>>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
>>>>> mptcp_established_options_add_addr.
>>>>>
>>>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
>>>>>> populate after the length chech, not before the check.]
>>>>>>
>>>>>> The squash-to patch keeped the more orignal code unchanged, and just do
>>>>>> the least, necessary modifications.
>>>>>>
>>>>> Agree opts->local and opts->remote should be asigned after the length check.
>>>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
>>>>> as orignal code, there is a race that:
>>>>>
>>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>>>> ==> call mptcp_pm_add_addr_signal
>>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
>>>>> ==> at this time opts->remote is empty and the length is incorrect.
>>>>>
>>>>
>>>> What will happen in v8 when this race occurs? How dose v8 deal with the
>>>> race?
>>> Hi Geliang, thinks for your patience.
>>>
>>> I think v8 doesn't have this issue:
>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
>>> ==> use add_addr and opts to check length.
>>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>>
>> Thanks for your explanation.
>>
>> I think this squash-to patch did the same thing:
>>
>> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
>> 'echo' (echo = false), save the port number to 'port', and save addr
>> in opts under pm.lock
>> ==> an echo add addr event trigger (pm.addr_signal ==
>> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
>> ==> use 'echo' to get the address family, use 'family', 'echo' and
>> 'port' to check length.
>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>>
>> Do you think so?
> 
> And one more thing. How do you test this "mptcp: fix conflicts when using
> pm.add_signal in ADD_ADDR/echo and RM_ADDR process" series? Do you use our
> mptcp_join.sh to test it, or you did some special tests? Does this race
> scenario mentioned above easy to reproduce?
> 
> I just did the mptcp_join.sh tests for my squash-to patches, and everything
> looks fine. If you have some special tests, could you please help me to test
> these squash-to patches too? Hope it works in the race scenario.
> 
> Thanks.
> -Geliang
> 
OK. I will try to test the squash-to patches.
The race scenario is not easy to reproduce. :(
A loop script will try many times to reproduce.

> 
> 
>>
>>>
>>>>
>>>>> So I think the orignal code is incorrect. WDYT?
>>>>>
>>>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>>>>>>
>>>>>> Change arguments of mptcp_pm_add_addr_signal.
>>>>>>
>>>>>> Keep mptcp_add_addr_len unchanged.
>>>>>>
>>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>>>>> ---
>>>>>>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
>>>>>>  net/mptcp/pm.c       | 23 +++++++++--------------
>>>>>>  net/mptcp/protocol.h | 27 +++++++++------------------
>>>>>>  3 files changed, 35 insertions(+), 50 deletions(-)
>>>>>>
>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>> index 5c0ad9b90866..93ad7b134f74 100644
>>>>>> --- a/net/mptcp/options.c
>>>>>> +++ b/net/mptcp/options.c
>>>>>> @@ -663,16 +663,14 @@ 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))
>>>>>> -             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))) &&
>>>>>> +     if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>> +          (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>> +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>>>           skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>               pr_debug("drop other suboptions");
>>>>>>               opts->suboptions = 0;
>>>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>               drop_other_suboptions = true;
>>>>>>       }
>>>>>>
>>>>>> -     len = mptcp_add_addr_len(opts, add_addr);
>>>>>> +     if (!mptcp_pm_should_add_signal(msk) ||
>>>>>> +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
>>>>>> +             return false;
>>>>>> +
>>>>>> +     family = echo ? opts->remote.family : opts->local.family;
>>>>>> +     len = mptcp_add_addr_len(family, echo, port);
>>>>>>       if (remaining < len)
>>>>>>               return false;
>>>>>>
>>>>>> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>       if (drop_other_suboptions)
>>>>>>               *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 +1255,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 264f522af530..399b59cb7563 100644
>>>>>> --- a/net/mptcp/pm.c
>>>>>> +++ b/net/mptcp/pm.c
>>>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>>
>>>>>>  /* path manager helpers */
>>>>>>
>>>>>> -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)
>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>>> +                           bool *echo, bool *port)
>>>>>>  {
>>>>>>       int ret = false;
>>>>>>       u8 add_addr;
>>>>>> +     u8 family;
>>>>>>
>>>>>>       spin_lock_bh(&msk->pm.lock);
>>>>>>
>>>>>> @@ -266,21 +267,15 @@ 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;
>>>>>> +     *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>>> +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>>>>>>
>>>>>> -     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;
>>>>>> -     }
>>>>>> -
>>>>>> -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
>>>>>> +     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;
>>>>>> +     *daddr = msk->pm.remote;
>>>>>>       add_addr = READ_ONCE(msk->pm.addr_signal);
>>>>>>       if (mptcp_pm_should_add_signal_echo(msk))
>>>>>>               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>> index 937e0309e340..4b63cc6079fa 100644
>>>>>> --- a/net/mptcp/protocol.h
>>>>>> +++ b/net/mptcp/protocol.h
>>>>>> @@ -766,25 +766,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(int 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;
>>>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
>>>>>> -                           unsigned int opt_size, unsigned int remaining,
>>>>>> -                           struct mptcp_out_options *opts,  u8 *add_addr);
>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>>> +                           bool *echo, bool *port);
>>>>>>  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
>>>>
>>>
>>> --
>>> Li YongLong
> 

-- 
Li YongLong

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

[Yonglong Li]

On 2021/7/12 16:44, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
>>
>>
>>
>> On 2021/7/12 15:33, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>>>>
>>>>
>>>>
>>>> On 2021/7/11 23:15, Geliang Tang wrote:
>>>>> I think there're still some issues in v8:
>>>>>
>>>>> The remaining value is incorrect since "remaining += opt_size;" in the
>>>>> "drop other suboptions" checks has been called twice in
>>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>>>>>
>>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
>>>> mptcp_established_options_add_addr.
>>>>
>>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
>>>>> populate after the length chech, not before the check.]
>>>>>
>>>>> The squash-to patch keeped the more orignal code unchanged, and just do
>>>>> the least, necessary modifications.
>>>>>
>>>> Agree opts->local and opts->remote should be asigned after the length check.
>>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
>>>> as orignal code, there is a race that:
>>>>
>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>>> ==> call mptcp_pm_add_addr_signal
>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
>>>> ==> at this time opts->remote is empty and the length is incorrect.
>>>>
>>>
>>> What will happen in v8 when this race occurs? How dose v8 deal with the
>>> race?
>> Hi Geliang, thinks for your patience.
>>
>> I think v8 doesn't have this issue:
>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
>> ==> use add_addr and opts to check length.
>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> 
> Thanks for your explanation.
> 
> I think this squash-to patch did the same thing:
> 
> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
> 'echo' (echo = false), save the port number to 'port', and save addr
> in opts under pm.lock
> ==> an echo add addr event trigger (pm.addr_signal ==
> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
> ==> use 'echo' to get the address family, use 'family', 'echo' and
> 'port' to check length.
> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> 
> Do you think so?
yep. In this case the squash-to patch is ok. But I think between "drop other suboptions" checks and
mptcp_pm_add_addr_signal the race still exist.

==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
==> "drop other suboptions" checks will use MPTCP_ADD_ADDR_SIGNAL to check
==> an echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL | MPTCP_ADD_ADDR_ECHO )
==> call mptcp_pm_add_addr_signal, MPTCP_ADD_ADDR_ECHO will be clear in pm.addr_signal
==> process MPTCP_ADD_ADDR_ECHO event.

WDYT?

> 
>>
>>>
>>>> So I think the orignal code is incorrect. WDYT?
>>>>
>>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>>>>>
>>>>> Change arguments of mptcp_pm_add_addr_signal.
>>>>>
>>>>> Keep mptcp_add_addr_len unchanged.
>>>>>
>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>>>> ---
>>>>>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
>>>>>  net/mptcp/pm.c       | 23 +++++++++--------------
>>>>>  net/mptcp/protocol.h | 27 +++++++++------------------
>>>>>  3 files changed, 35 insertions(+), 50 deletions(-)
>>>>>
>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>> index 5c0ad9b90866..93ad7b134f74 100644
>>>>> --- a/net/mptcp/options.c
>>>>> +++ b/net/mptcp/options.c
>>>>> @@ -663,16 +663,14 @@ 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))
>>>>> -             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))) &&
>>>>> +     if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>> +          (mptcp_pm_should_add_signal_addr(msk) &&
>>>>> +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>>           skb && skb_is_tcp_pure_ack(skb)) {
>>>>>               pr_debug("drop other suboptions");
>>>>>               opts->suboptions = 0;
>>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>               drop_other_suboptions = true;
>>>>>       }
>>>>>
>>>>> -     len = mptcp_add_addr_len(opts, add_addr);
>>>>> +     if (!mptcp_pm_should_add_signal(msk) ||
>>>>> +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
>>>>> +             return false;
>>>>> +
>>>>> +     family = echo ? opts->remote.family : opts->local.family;
>>>>> +     len = mptcp_add_addr_len(family, echo, port);
>>>>>       if (remaining < len)
>>>>>               return false;
>>>>>
>>>>> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>       if (drop_other_suboptions)
>>>>>               *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 +1255,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 264f522af530..399b59cb7563 100644
>>>>> --- a/net/mptcp/pm.c
>>>>> +++ b/net/mptcp/pm.c
>>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>
>>>>>  /* path manager helpers */
>>>>>
>>>>> -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)
>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>> +                           bool *echo, bool *port)
>>>>>  {
>>>>>       int ret = false;
>>>>>       u8 add_addr;
>>>>> +     u8 family;
>>>>>
>>>>>       spin_lock_bh(&msk->pm.lock);
>>>>>
>>>>> @@ -266,21 +267,15 @@ 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;
>>>>> +     *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>> +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>>>>>
>>>>> -     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;
>>>>> -     }
>>>>> -
>>>>> -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
>>>>> +     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;
>>>>> +     *daddr = msk->pm.remote;
>>>>>       add_addr = READ_ONCE(msk->pm.addr_signal);
>>>>>       if (mptcp_pm_should_add_signal_echo(msk))
>>>>>               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>> index 937e0309e340..4b63cc6079fa 100644
>>>>> --- a/net/mptcp/protocol.h
>>>>> +++ b/net/mptcp/protocol.h
>>>>> @@ -766,25 +766,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(int 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;
>>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
>>>>> -                           unsigned int opt_size, unsigned int remaining,
>>>>> -                           struct mptcp_out_options *opts,  u8 *add_addr);
>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>> +                           bool *echo, bool *port);
>>>>>  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
>>>
>>
>> --
>> Li YongLong
> 
> 

-- 
Li YongLong

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

[Geliang Tang]

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:14写道：
>
>
>
> On 2021/7/12 16:44, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
> >>
> >>
> >>
> >> On 2021/7/12 15:33, Geliang Tang wrote:
> >>> Hi Yonglong,
> >>>
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
> >>>>
> >>>>
> >>>>
> >>>> On 2021/7/11 23:15, Geliang Tang wrote:
> >>>>> I think there're still some issues in v8:
> >>>>>
> >>>>> The remaining value is incorrect since "remaining += opt_size;" in the
> >>>>> "drop other suboptions" checks has been called twice in
> >>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> >>>>>
> >>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> >>>> mptcp_established_options_add_addr.
> >>>>
> >>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> >>>>> populate after the length chech, not before the check.]
> >>>>>
> >>>>> The squash-to patch keeped the more orignal code unchanged, and just do
> >>>>> the least, necessary modifications.
> >>>>>
> >>>> Agree opts->local and opts->remote should be asigned after the length check.
> >>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> >>>> as orignal code, there is a race that:
> >>>>
> >>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >>>> ==> call mptcp_pm_add_addr_signal
> >>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> >>>> ==> at this time opts->remote is empty and the length is incorrect.
> >>>>
> >>>
> >>> What will happen in v8 when this race occurs? How dose v8 deal with the
> >>> race?
> >> Hi Geliang, thinks for your patience.
> >>
> >> I think v8 doesn't have this issue:
> >> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
> >> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
> >> ==> use add_addr and opts to check length.
> >> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> >
> > Thanks for your explanation.
> >
> > I think this squash-to patch did the same thing:
> >
> > ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> > ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
> > 'echo' (echo = false), save the port number to 'port', and save addr
> > in opts under pm.lock
> > ==> an echo add addr event trigger (pm.addr_signal ==
> > MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
> > ==> use 'echo' to get the address family, use 'family', 'echo' and
> > 'port' to check length.
> > ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> >
> > Do you think so?
> yep. In this case the squash-to patch is ok. But I think between "drop other suboptions" checks and
> mptcp_pm_add_addr_signal the race still exist.
>

I think this is easy to fix:

Add a new argument "drop_other_suboptions" for mptcp_pm_add_addr_signal,
move this "drop other suboptions" check code into mptcp_pm_add_addr_signal,
I'll sent a v2 later.

Thanks,
-Geliang

> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> "drop other suboptions" checks will use MPTCP_ADD_ADDR_SIGNAL to check
> ==> an echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL | MPTCP_ADD_ADDR_ECHO )
> ==> call mptcp_pm_add_addr_signal, MPTCP_ADD_ADDR_ECHO will be clear in pm.addr_signal
> ==> process MPTCP_ADD_ADDR_ECHO event.
>
> WDYT?
>
> >
> >>
> >>>
> >>>> So I think the orignal code is incorrect. WDYT?
> >>>>
> >>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> >>>>>
> >>>>> Change arguments of mptcp_pm_add_addr_signal.
> >>>>>
> >>>>> Keep mptcp_add_addr_len unchanged.
> >>>>>
> >>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> >>>>> ---
> >>>>>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
> >>>>>  net/mptcp/pm.c       | 23 +++++++++--------------
> >>>>>  net/mptcp/protocol.h | 27 +++++++++------------------
> >>>>>  3 files changed, 35 insertions(+), 50 deletions(-)
> >>>>>
> >>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>> index 5c0ad9b90866..93ad7b134f74 100644
> >>>>> --- a/net/mptcp/options.c
> >>>>> +++ b/net/mptcp/options.c
> >>>>> @@ -663,16 +663,14 @@ 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))
> >>>>> -             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))) &&
> >>>>> +     if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>>> +          (mptcp_pm_should_add_signal_addr(msk) &&
> >>>>> +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>>>>           skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>               pr_debug("drop other suboptions");
> >>>>>               opts->suboptions = 0;
> >>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>               drop_other_suboptions = true;
> >>>>>       }
> >>>>>
> >>>>> -     len = mptcp_add_addr_len(opts, add_addr);
> >>>>> +     if (!mptcp_pm_should_add_signal(msk) ||
> >>>>> +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> >>>>> +             return false;
> >>>>> +
> >>>>> +     family = echo ? opts->remote.family : opts->local.family;
> >>>>> +     len = mptcp_add_addr_len(family, echo, port);
> >>>>>       if (remaining < len)
> >>>>>               return false;
> >>>>>
> >>>>> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>       if (drop_other_suboptions)
> >>>>>               *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 +1255,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 264f522af530..399b59cb7563 100644
> >>>>> --- a/net/mptcp/pm.c
> >>>>> +++ b/net/mptcp/pm.c
> >>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>>>
> >>>>>  /* path manager helpers */
> >>>>>
> >>>>> -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)
> >>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>>>> +                           bool *echo, bool *port)
> >>>>>  {
> >>>>>       int ret = false;
> >>>>>       u8 add_addr;
> >>>>> +     u8 family;
> >>>>>
> >>>>>       spin_lock_bh(&msk->pm.lock);
> >>>>>
> >>>>> @@ -266,21 +267,15 @@ 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;
> >>>>> +     *echo = mptcp_pm_should_add_signal_echo(msk);
> >>>>> +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> >>>>>
> >>>>> -     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;
> >>>>> -     }
> >>>>> -
> >>>>> -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
> >>>>> +     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;
> >>>>> +     *daddr = msk->pm.remote;
> >>>>>       add_addr = READ_ONCE(msk->pm.addr_signal);
> >>>>>       if (mptcp_pm_should_add_signal_echo(msk))
> >>>>>               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>>>> index 937e0309e340..4b63cc6079fa 100644
> >>>>> --- a/net/mptcp/protocol.h
> >>>>> +++ b/net/mptcp/protocol.h
> >>>>> @@ -766,25 +766,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(int 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;
> >>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> >>>>> -                           unsigned int opt_size, unsigned int remaining,
> >>>>> -                           struct mptcp_out_options *opts,  u8 *add_addr);
> >>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>>>> +                           bool *echo, bool *port);
> >>>>>  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
> >>>
> >>
> >> --
> >> Li YongLong
> >
> >
>
> --
> Li YongLong

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

[Yonglong Li]

On 2021/7/12 17:29, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:14写道：
>>
>>
>>
>> On 2021/7/12 16:44, Geliang Tang wrote:
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
>>>>
>>>>
>>>>
>>>> On 2021/7/12 15:33, Geliang Tang wrote:
>>>>> Hi Yonglong,
>>>>>
>>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2021/7/11 23:15, Geliang Tang wrote:
>>>>>>> I think there're still some issues in v8:
>>>>>>>
>>>>>>> The remaining value is incorrect since "remaining += opt_size;" in the
>>>>>>> "drop other suboptions" checks has been called twice in
>>>>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>>>>>>>
>>>>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
>>>>>> mptcp_established_options_add_addr.
>>>>>>
>>>>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
>>>>>>> populate after the length chech, not before the check.]
>>>>>>>
>>>>>>> The squash-to patch keeped the more orignal code unchanged, and just do
>>>>>>> the least, necessary modifications.
>>>>>>>
>>>>>> Agree opts->local and opts->remote should be asigned after the length check.
>>>>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
>>>>>> as orignal code, there is a race that:
>>>>>>
>>>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>>>>> ==> call mptcp_pm_add_addr_signal
>>>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
>>>>>> ==> at this time opts->remote is empty and the length is incorrect.
>>>>>>
>>>>>
>>>>> What will happen in v8 when this race occurs? How dose v8 deal with the
>>>>> race?
>>>> Hi Geliang, thinks for your patience.
>>>>
>>>> I think v8 doesn't have this issue:
>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>>> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
>>>> ==> use add_addr and opts to check length.
>>>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>>>
>>> Thanks for your explanation.
>>>
>>> I think this squash-to patch did the same thing:
>>>
>>> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
>>> 'echo' (echo = false), save the port number to 'port', and save addr
>>> in opts under pm.lock
>>> ==> an echo add addr event trigger (pm.addr_signal ==
>>> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
>>> ==> use 'echo' to get the address family, use 'family', 'echo' and
>>> 'port' to check length.
>>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>>>
>>> Do you think so?
>> yep. In this case the squash-to patch is ok. But I think between "drop other suboptions" checks and
>> mptcp_pm_add_addr_signal the race still exist.
>>
> 
> I think this is easy to fix:
> 
> Add a new argument "drop_other_suboptions" for mptcp_pm_add_addr_signal,
> move this "drop other suboptions" check code into mptcp_pm_add_addr_signal,
> I'll sent a v2 later.

Thanks. And the v8 do the same thing. Why not use v8 directly :)

> 
> Thanks,
> -Geliang
> 
>> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>> ==> "drop other suboptions" checks will use MPTCP_ADD_ADDR_SIGNAL to check
>> ==> an echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL | MPTCP_ADD_ADDR_ECHO )
>> ==> call mptcp_pm_add_addr_signal, MPTCP_ADD_ADDR_ECHO will be clear in pm.addr_signal
>> ==> process MPTCP_ADD_ADDR_ECHO event.
>>
>> WDYT?
>>
>>>
>>>>
>>>>>
>>>>>> So I think the orignal code is incorrect. WDYT?
>>>>>>
>>>>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>>>>>>>
>>>>>>> Change arguments of mptcp_pm_add_addr_signal.
>>>>>>>
>>>>>>> Keep mptcp_add_addr_len unchanged.
>>>>>>>
>>>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>>>>>> ---
>>>>>>>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
>>>>>>>  net/mptcp/pm.c       | 23 +++++++++--------------
>>>>>>>  net/mptcp/protocol.h | 27 +++++++++------------------
>>>>>>>  3 files changed, 35 insertions(+), 50 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>>> index 5c0ad9b90866..93ad7b134f74 100644
>>>>>>> --- a/net/mptcp/options.c
>>>>>>> +++ b/net/mptcp/options.c
>>>>>>> @@ -663,16 +663,14 @@ 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))
>>>>>>> -             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))) &&
>>>>>>> +     if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>>> +          (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>>> +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>>>>           skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>>               pr_debug("drop other suboptions");
>>>>>>>               opts->suboptions = 0;
>>>>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>>               drop_other_suboptions = true;
>>>>>>>       }
>>>>>>>
>>>>>>> -     len = mptcp_add_addr_len(opts, add_addr);
>>>>>>> +     if (!mptcp_pm_should_add_signal(msk) ||
>>>>>>> +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
>>>>>>> +             return false;
>>>>>>> +
>>>>>>> +     family = echo ? opts->remote.family : opts->local.family;
>>>>>>> +     len = mptcp_add_addr_len(family, echo, port);
>>>>>>>       if (remaining < len)
>>>>>>>               return false;
>>>>>>>
>>>>>>> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>>       if (drop_other_suboptions)
>>>>>>>               *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 +1255,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 264f522af530..399b59cb7563 100644
>>>>>>> --- a/net/mptcp/pm.c
>>>>>>> +++ b/net/mptcp/pm.c
>>>>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>>>
>>>>>>>  /* path manager helpers */
>>>>>>>
>>>>>>> -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)
>>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>>>> +                           bool *echo, bool *port)
>>>>>>>  {
>>>>>>>       int ret = false;
>>>>>>>       u8 add_addr;
>>>>>>> +     u8 family;
>>>>>>>
>>>>>>>       spin_lock_bh(&msk->pm.lock);
>>>>>>>
>>>>>>> @@ -266,21 +267,15 @@ 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;
>>>>>>> +     *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>>>> +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>>>>>>>
>>>>>>> -     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;
>>>>>>> -     }
>>>>>>> -
>>>>>>> -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
>>>>>>> +     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;
>>>>>>> +     *daddr = msk->pm.remote;
>>>>>>>       add_addr = READ_ONCE(msk->pm.addr_signal);
>>>>>>>       if (mptcp_pm_should_add_signal_echo(msk))
>>>>>>>               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>>> index 937e0309e340..4b63cc6079fa 100644
>>>>>>> --- a/net/mptcp/protocol.h
>>>>>>> +++ b/net/mptcp/protocol.h
>>>>>>> @@ -766,25 +766,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(int 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;
>>>>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
>>>>>>> -                           unsigned int opt_size, unsigned int remaining,
>>>>>>> -                           struct mptcp_out_options *opts,  u8 *add_addr);
>>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>>>> +                           bool *echo, bool *port);
>>>>>>>  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
>>>>>
>>>>
>>>> --
>>>> Li YongLong
>>>
>>>
>>
>> --
>> Li YongLong
> 

-- 
Li YongLong

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

[Geliang Tang]

Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:44写道：
>
>
>
> On 2021/7/12 17:29, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:14写道：
> >>
> >>
> >>
> >> On 2021/7/12 16:44, Geliang Tang wrote:
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
> >>>>
> >>>>
> >>>>
> >>>> On 2021/7/12 15:33, Geliang Tang wrote:
> >>>>> Hi Yonglong,
> >>>>>
> >>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2021/7/11 23:15, Geliang Tang wrote:
> >>>>>>> I think there're still some issues in v8:
> >>>>>>>
> >>>>>>> The remaining value is incorrect since "remaining += opt_size;" in the
> >>>>>>> "drop other suboptions" checks has been called twice in
> >>>>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> >>>>>>>
> >>>>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> >>>>>> mptcp_established_options_add_addr.
> >>>>>>
> >>>>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> >>>>>>> populate after the length chech, not before the check.]
> >>>>>>>
> >>>>>>> The squash-to patch keeped the more orignal code unchanged, and just do
> >>>>>>> the least, necessary modifications.
> >>>>>>>
> >>>>>> Agree opts->local and opts->remote should be asigned after the length check.
> >>>>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> >>>>>> as orignal code, there is a race that:
> >>>>>>
> >>>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >>>>>> ==> call mptcp_pm_add_addr_signal
> >>>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> >>>>>> ==> at this time opts->remote is empty and the length is incorrect.
> >>>>>>
> >>>>>
> >>>>> What will happen in v8 when this race occurs? How dose v8 deal with the
> >>>>> race?
> >>>> Hi Geliang, thinks for your patience.
> >>>>
> >>>> I think v8 doesn't have this issue:
> >>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >>>> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
> >>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
> >>>> ==> use add_addr and opts to check length.
> >>>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> >>>
> >>> Thanks for your explanation.
> >>>
> >>> I think this squash-to patch did the same thing:
> >>>
> >>> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >>> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
> >>> 'echo' (echo = false), save the port number to 'port', and save addr
> >>> in opts under pm.lock
> >>> ==> an echo add addr event trigger (pm.addr_signal ==
> >>> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
> >>> ==> use 'echo' to get the address family, use 'family', 'echo' and
> >>> 'port' to check length.
> >>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> >>>
> >>> Do you think so?
> >> yep. In this case the squash-to patch is ok. But I think between "drop other suboptions" checks and
> >> mptcp_pm_add_addr_signal the race still exist.
> >>
> >
> > I think this is easy to fix:
> >
> > Add a new argument "drop_other_suboptions" for mptcp_pm_add_addr_signal,
> > move this "drop other suboptions" check code into mptcp_pm_add_addr_signal,
> > I'll sent a v2 later.
>
> Thanks. And the v8 do the same thing. Why not use v8 directly :)
>

You'll see the difference later. :)

> >
> > Thanks,
> > -Geliang
> >
> >> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >> ==> "drop other suboptions" checks will use MPTCP_ADD_ADDR_SIGNAL to check
> >> ==> an echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL | MPTCP_ADD_ADDR_ECHO )
> >> ==> call mptcp_pm_add_addr_signal, MPTCP_ADD_ADDR_ECHO will be clear in pm.addr_signal
> >> ==> process MPTCP_ADD_ADDR_ECHO event.
> >>
> >> WDYT?
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> So I think the orignal code is incorrect. WDYT?
> >>>>>>
> >>>>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> >>>>>>>
> >>>>>>> Change arguments of mptcp_pm_add_addr_signal.
> >>>>>>>
> >>>>>>> Keep mptcp_add_addr_len unchanged.
> >>>>>>>
> >>>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> >>>>>>> ---
> >>>>>>>  net/mptcp/options.c  | 35 +++++++++++++++++------------------
> >>>>>>>  net/mptcp/pm.c       | 23 +++++++++--------------
> >>>>>>>  net/mptcp/protocol.h | 27 +++++++++------------------
> >>>>>>>  3 files changed, 35 insertions(+), 50 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>>>> index 5c0ad9b90866..93ad7b134f74 100644
> >>>>>>> --- a/net/mptcp/options.c
> >>>>>>> +++ b/net/mptcp/options.c
> >>>>>>> @@ -663,16 +663,14 @@ 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))
> >>>>>>> -             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))) &&
> >>>>>>> +     if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>>>>> +          (mptcp_pm_should_add_signal_addr(msk) &&
> >>>>>>> +           (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>>>>>>           skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>>>               pr_debug("drop other suboptions");
> >>>>>>>               opts->suboptions = 0;
> >>>>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>>>               drop_other_suboptions = true;
> >>>>>>>       }
> >>>>>>>
> >>>>>>> -     len = mptcp_add_addr_len(opts, add_addr);
> >>>>>>> +     if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>>> +         !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> >>>>>>> +             return false;
> >>>>>>> +
> >>>>>>> +     family = echo ? opts->remote.family : opts->local.family;
> >>>>>>> +     len = mptcp_add_addr_len(family, echo, port);
> >>>>>>>       if (remaining < len)
> >>>>>>>               return false;
> >>>>>>>
> >>>>>>> @@ -690,15 +693,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>>>       if (drop_other_suboptions)
> >>>>>>>               *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 +1255,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 264f522af530..399b59cb7563 100644
> >>>>>>> --- a/net/mptcp/pm.c
> >>>>>>> +++ b/net/mptcp/pm.c
> >>>>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>>>>>
> >>>>>>>  /* path manager helpers */
> >>>>>>>
> >>>>>>> -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)
> >>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>>>>>> +                           bool *echo, bool *port)
> >>>>>>>  {
> >>>>>>>       int ret = false;
> >>>>>>>       u8 add_addr;
> >>>>>>> +     u8 family;
> >>>>>>>
> >>>>>>>       spin_lock_bh(&msk->pm.lock);
> >>>>>>>
> >>>>>>> @@ -266,21 +267,15 @@ 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;
> >>>>>>> +     *echo = mptcp_pm_should_add_signal_echo(msk);
> >>>>>>> +     *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> >>>>>>>
> >>>>>>> -     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;
> >>>>>>> -     }
> >>>>>>> -
> >>>>>>> -     if (remaining < mptcp_add_addr_len(opts, *add_addr))
> >>>>>>> +     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;
> >>>>>>> +     *daddr = msk->pm.remote;
> >>>>>>>       add_addr = READ_ONCE(msk->pm.addr_signal);
> >>>>>>>       if (mptcp_pm_should_add_signal_echo(msk))
> >>>>>>>               add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>>>>>> index 937e0309e340..4b63cc6079fa 100644
> >>>>>>> --- a/net/mptcp/protocol.h
> >>>>>>> +++ b/net/mptcp/protocol.h
> >>>>>>> @@ -766,25 +766,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(int 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;
> >>>>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> >>>>>>> -                           unsigned int opt_size, unsigned int remaining,
> >>>>>>> -                           struct mptcp_out_options *opts,  u8 *add_addr);
> >>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>>> +                           struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>>>>>> +                           bool *echo, bool *port);
> >>>>>>>  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
> >>>>>
> >>>>
> >>>> --
> >>>> Li YongLong
> >>>
> >>>
> >>
> >> --
> >> Li YongLong
> >
>
> --
> Li YongLong

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate"

[Mat Martineau]

On Sun, 11 Jul 2021, Geliang Tang wrote:

> A small cleanup.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/pm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 30deb76fa5d0..1eeecd68f159 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -18,7 +18,7 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> {
> 	u8 add_addr = READ_ONCE(msk->pm.addr_signal);
>
> -	pr_debug("msk=%p, local_id=%d, echo:%d", msk, addr->id, echo);
> +	pr_debug("msk=%p, local_id=%d, echo=%d", msk, addr->id, echo);
>
> 	lockdep_assert_held(&msk->pm.lock);
>
> -- 
> 2.31.1

Thanks for catching this detail!

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel