[v1] mptcp: send ADD_ADDR echo before create subflows

[PATCH] mptcp: send ADD_ADDR echo before create subflows

Posted by Yonglong Li 2 years, 2 months ago

In some corner case, It may receive ADD_ADDR retransmit package
before created subflows done.
It's more reasonable to send ADD_ADDR echo before creating subflows.

Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/pm_netlink.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e3b0384..c1f4bef 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -650,7 +650,6 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	unsigned int add_addr_accept_max;
 	struct mptcp_addr_info remote;
 	unsigned int subflows_max;
-	bool reset_port = false;
 	int i, nr;
 
 	add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
@@ -661,14 +660,15 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 		 msk->pm.remote.family);
 
 	remote = msk->pm.remote;
+	mptcp_pm_announce_addr(msk, &remote, true);
+	mptcp_pm_nl_addr_send_ack(msk);
+
 	if (lookup_subflow_by_daddr(&msk->conn_list, &remote))
-		goto add_addr_echo;
+		return;
 
 	/* pick id 0 port, if none is provided the remote address */
-	if (!remote.port) {
-		reset_port = true;
+	if (!remote.port)
 		remote.port = sk->sk_dport;
-	}
 
 	/* connect to the specified remote address, using whatever
 	 * local address the routing configuration will pick.
@@ -684,14 +684,6 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	for (i = 0; i < nr; i++)
 		__mptcp_subflow_connect(sk, &addrs[i], &remote);
 	spin_lock_bh(&msk->pm.lock);
-
-	/* be sure to echo exactly the received address */
-	if (reset_port)
-		remote.port = 0;
-
-add_addr_echo:
-	mptcp_pm_announce_addr(msk, &remote, true);
-	mptcp_pm_nl_addr_send_ack(msk);
 }
 
 void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
-- 
1.8.3.1

Re: mptcp: send ADD_ADDR echo before create subflows: Tests Results

Posted by MPTCP CI 2 years, 2 months ago

Hi Yonglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 2 failed test(s): packetdrill_mp_join packetdrill_sockopts 🔴:
  - Task: https://cirrus-ci.com/task/4939098853998592
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4939098853998592/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 2 failed test(s): packetdrill_mp_join packetdrill_sockopts 🔴:
  - Task: https://cirrus-ci.com/task/6064998760841216
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6064998760841216/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/54cb4805b853

Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH] mptcp: send ADD_ADDR echo before create subflows

Posted by Yonglong Li 2 years, 1 month ago

I have have send corresponding pull-request to packdrill:
https://github.com/multipath-tcp/packetdrill/pull/80

On 2/25/2022 4:53 PM, Yonglong Li wrote:
> In some corner case, It may receive ADD_ADDR retransmit package
> before created subflows done.
> It's more reasonable to send ADD_ADDR echo before creating subflows.
> 
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
>  net/mptcp/pm_netlink.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index e3b0384..c1f4bef 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -650,7 +650,6 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  	unsigned int add_addr_accept_max;
>  	struct mptcp_addr_info remote;
>  	unsigned int subflows_max;
> -	bool reset_port = false;
>  	int i, nr;
>  
>  	add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
> @@ -661,14 +660,15 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  		 msk->pm.remote.family);
>  
>  	remote = msk->pm.remote;
> +	mptcp_pm_announce_addr(msk, &remote, true);
> +	mptcp_pm_nl_addr_send_ack(msk);
> +
>  	if (lookup_subflow_by_daddr(&msk->conn_list, &remote))
> -		goto add_addr_echo;
> +		return;
>  
>  	/* pick id 0 port, if none is provided the remote address */
> -	if (!remote.port) {
> -		reset_port = true;
> +	if (!remote.port)
>  		remote.port = sk->sk_dport;
> -	}
>  
>  	/* connect to the specified remote address, using whatever
>  	 * local address the routing configuration will pick.
> @@ -684,14 +684,6 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  	for (i = 0; i < nr; i++)
>  		__mptcp_subflow_connect(sk, &addrs[i], &remote);
>  	spin_lock_bh(&msk->pm.lock);
> -
> -	/* be sure to echo exactly the received address */
> -	if (reset_port)
> -		remote.port = 0;
> -
> -add_addr_echo:
> -	mptcp_pm_announce_addr(msk, &remote, true);
> -	mptcp_pm_nl_addr_send_ack(msk);
>  }
>  
>  void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
> 

-- 
Li YongLong

Re: [PATCH] mptcp: send ADD_ADDR echo before create subflows

Posted by Matthieu Baerts 2 years, 1 month ago

Hi Yonglong, Paolo

On 25/02/2022 09:53, Yonglong Li wrote:
> In some corner case, It may receive ADD_ADDR retransmit package
> before created subflows done.
> It's more reasonable to send ADD_ADDR echo before creating subflows.

Thank you for this patch and the review!

Applied in our tree (feat. for net-next) with Paolo's ACK:

- 2b7de9309fe0: mptcp: send ADD_ADDR echo before create subflows
- Results: 097423b70867..64f0c8f3cf8b

This patch could be backported but as mentioned by Paolo, this fix is
more an optimisation and it changes the behaviour.

Builds and tests are now in progress:

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

Note: packetdrill tests have been adapted to this new behaviour.

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

Re: [PATCH] mptcp: send ADD_ADDR echo before create subflows

Posted by Paolo Abeni 2 years, 1 month ago

Hello,

On Fri, 2022-02-25 at 16:53 +0800, Yonglong Li wrote:
> In some corner case, It may receive ADD_ADDR retransmit package
> before created subflows done.
> It's more reasonable to send ADD_ADDR echo before creating subflows.
> 
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>

The patch LGTM, and beyond fixing some corner case it looks like a nice
cleanup.

I think the changelog could be clairified a bit (even if I'm possibly
the last person to give this specific kind of adivice ;). What about
something alike:

"""
In some corner cases, the peer handing an incoming ADD_ADDR option, can
receive a retransmitted ADD_ADDR for the same address before the
subflow creation completes.

We can avoid the above issue by generating and sending the ADD_ADDR
echo before starting the MPJ subflow connection.
"""

@Matt could you handle such change while applaying the patch?

Thanks!

Re: [PATCH] mptcp: send ADD_ADDR echo before create subflows

Posted by Matthieu Baerts 2 years, 1 month ago

Hi Paolo,

On 01/03/2022 10:15, Paolo Abeni wrote:
> Hello,
> 
> On Fri, 2022-02-25 at 16:53 +0800, Yonglong Li wrote:
>> In some corner case, It may receive ADD_ADDR retransmit package
>> before created subflows done.
>> It's more reasonable to send ADD_ADDR echo before creating subflows.
>>
>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> 
> The patch LGTM, and beyond fixing some corner case it looks like a nice
> cleanup.

Can I add your Acked-by? :)

> I think the changelog could be clairified a bit (even if I'm possibly
> the last person to give this specific kind of adivice ;). What about
> something alike:
> 
> """
> In some corner cases, the peer handing an incoming ADD_ADDR option, can
> receive a retransmitted ADD_ADDR for the same address before the
> subflow creation completes.
> 
> We can avoid the above issue by generating and sending the ADD_ADDR
> echo before starting the MPJ subflow connection.
> """
Good idea! Indeed clearer with this updated commit message.

> @Matt could you handle such change while applaying the patch?

Sure I can if there is no objection from Yonglong!

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