[v2] mptcp: don't orphan ssk in mptcp_close()

[PATCH mptcp-net v2] mptcp: don't orphan ssk in mptcp_close()

Posted by menglong8.dong@gmail.com 1 year, 5 months ago

From: Menglong Dong <imagedong@tencent.com>

All of the subflows of a msk will be orphaned in mptcp_close(), which
means the subflows are in DEAD state. After then, DATA_FIN will be sent,
and the other side will response with a DATA_ACK for this DATA_FIN.

However, if the other side still has pending data, the data that received
on these subflows will not be passed to the msk, as they are DEAD and
subflow_data_ready() will not be called in tcp_data_ready(). Therefore,
these data can't be acked, and they will be retransmitted again and again,
until timeout.

Fix this by setting ssk->sk_socket and ssk->sk_wq to 'NULL', instead of
orphaning the subflows in __mptcp_close(), as Paolo suggested.

Reviewed-by: Biao Jiang <benbjiang@tencent.com>
Reviewed-by: Mengen Sun <mengensun@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v2:
- setting ssk->sk_socket and ssk->sk_wq to 'NULL' in __mptcp_close(),
  as Paolo suggested.
- remove the unnecessary 'SOCK_DEAD' check in mptcp_data_ready()
---
 net/mptcp/protocol.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3796d1bfef6b..a4fd971a7aa4 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2352,12 +2352,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 		goto out;
 	}
 
-	/* if we are invoked by the msk cleanup code, the subflow is
-	 * already orphaned
-	 */
-	if (ssk->sk_socket)
-		sock_orphan(ssk);
-
+	sock_orphan(ssk);
 	subflow->disposable = 1;
 
 	/* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops
@@ -2940,7 +2935,11 @@ bool __mptcp_close(struct sock *sk, long timeout)
 		if (ssk == msk->first)
 			subflow->fail_tout = 0;
 
-		sock_orphan(ssk);
+		/* detach from the parent socket, but allow data_ready to
+		 * push incoming date into the mptcp stack, to properly ack it
+		 */
+		ssk->sk_socket = NULL;
+		ssk->sk_wq = NULL;
 		unlock_sock_fast(ssk, slow);
 	}
 	sock_orphan(sk);
-- 
2.37.2

Re: mptcp: don't orphan ssk in mptcp_close(): Tests Results

Posted by MPTCP CI 1 year, 5 months ago

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5471374876606464
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5471374876606464/summary/summary.txt

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6597274783449088
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6597274783449088/summary/summary.txt

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


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-net v2] mptcp: don't orphan ssk in mptcp_close()

Posted by Matthieu Baerts 1 year, 5 months ago

Hi Menglong, Paolo,

On 21/11/2022 04:41, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> All of the subflows of a msk will be orphaned in mptcp_close(), which
> means the subflows are in DEAD state. After then, DATA_FIN will be sent,
> and the other side will response with a DATA_ACK for this DATA_FIN.
> 
> However, if the other side still has pending data, the data that received
> on these subflows will not be passed to the msk, as they are DEAD and
> subflow_data_ready() will not be called in tcp_data_ready(). Therefore,
> these data can't be acked, and they will be retransmitted again and again,
> until timeout.
> 
> Fix this by setting ssk->sk_socket and ssk->sk_wq to 'NULL', instead of
> orphaning the subflows in __mptcp_close(), as Paolo suggested.
> 
> Reviewed-by: Biao Jiang <benbjiang@tencent.com>
> Reviewed-by: Mengen Sun <mengensun@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>

Thank you for the patch and the review!

Now in our tree (fix for -net) with Paolo's RvB tag, a Fixes tag and
without the typo (s/date/data/):

- e8695e504942: mptcp: don't orphan ssk in mptcp_close()
- Results: 09c91a08d1dd..f5e72ede5629 (export-net)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/

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

Re: [PATCH mptcp-net v2] mptcp: don't orphan ssk in mptcp_close()

Posted by Paolo Abeni 1 year, 5 months ago

On Mon, 2022-11-21 at 11:41 +0800, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> All of the subflows of a msk will be orphaned in mptcp_close(), which
> means the subflows are in DEAD state. After then, DATA_FIN will be sent,
> and the other side will response with a DATA_ACK for this DATA_FIN.
> 
> However, if the other side still has pending data, the data that received
> on these subflows will not be passed to the msk, as they are DEAD and
> subflow_data_ready() will not be called in tcp_data_ready(). Therefore,
> these data can't be acked, and they will be retransmitted again and again,
> until timeout.
> 
> Fix this by setting ssk->sk_socket and ssk->sk_wq to 'NULL', instead of
> orphaning the subflows in __mptcp_close(), as Paolo suggested.
> 
> Reviewed-by: Biao Jiang <benbjiang@tencent.com>
> Reviewed-by: Mengen Sun <mengensun@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>

Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
Reviewed-by: Paolo Abeni <pabeni@redhat.com>

@Mat: could you please add even the fixes tag when applying this one?

> ---
> v2:
> - setting ssk->sk_socket and ssk->sk_wq to 'NULL' in __mptcp_close(),
>   as Paolo suggested.
> - remove the unnecessary 'SOCK_DEAD' check in mptcp_data_ready()
> ---
>  net/mptcp/protocol.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3796d1bfef6b..a4fd971a7aa4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2352,12 +2352,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		goto out;
>  	}
>  
> -	/* if we are invoked by the msk cleanup code, the subflow is
> -	 * already orphaned
> -	 */
> -	if (ssk->sk_socket)
> -		sock_orphan(ssk);
> -
> +	sock_orphan(ssk);
>  	subflow->disposable = 1;
>  
>  	/* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops
> @@ -2940,7 +2935,11 @@ bool __mptcp_close(struct sock *sk, long timeout)
>  		if (ssk == msk->first)
>  			subflow->fail_tout = 0;
>  
> -		sock_orphan(ssk);
> +		/* detach from the parent socket, but allow data_ready to
> +		 * push incoming date into the mptcp stack, to properly ack it

Nit:                             ^^^^

should be 'data'

@Mat: could you please fix it while applying or do you prefer a v3 or a
squash-to patch?

Thanks!

Paolo

Re: [PATCH mptcp-net v2] mptcp: don't orphan ssk in mptcp_close()

Posted by Matthieu Baerts 1 year, 5 months ago

Hi Paolo,

On 21/11/2022 11:03, Paolo Abeni wrote:
> On Mon, 2022-11-21 at 11:41 +0800, menglong8.dong@gmail.com wrote:
>> From: Menglong Dong <imagedong@tencent.com>
>>
>> All of the subflows of a msk will be orphaned in mptcp_close(), which
>> means the subflows are in DEAD state. After then, DATA_FIN will be sent,
>> and the other side will response with a DATA_ACK for this DATA_FIN.
>>
>> However, if the other side still has pending data, the data that received
>> on these subflows will not be passed to the msk, as they are DEAD and
>> subflow_data_ready() will not be called in tcp_data_ready(). Therefore,
>> these data can't be acked, and they will be retransmitted again and again,
>> until timeout.
>>
>> Fix this by setting ssk->sk_socket and ssk->sk_wq to 'NULL', instead of
>> orphaning the subflows in __mptcp_close(), as Paolo suggested.
>>
>> Reviewed-by: Biao Jiang <benbjiang@tencent.com>
>> Reviewed-by: Mengen Sun <mengensun@tencent.com>
>> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> 
> Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
> Reviewed-by: Paolo Abeni <pabeni@redhat.com>
> 
> @Mat: could you please add even the fixes tag when applying this one?

Sure! (b4 will add it automatically)

> 
>> ---
>> v2:
>> - setting ssk->sk_socket and ssk->sk_wq to 'NULL' in __mptcp_close(),
>>   as Paolo suggested.
>> - remove the unnecessary 'SOCK_DEAD' check in mptcp_data_ready()
>> ---
>>  net/mptcp/protocol.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 3796d1bfef6b..a4fd971a7aa4 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -2352,12 +2352,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>>  		goto out;
>>  	}
>>  
>> -	/* if we are invoked by the msk cleanup code, the subflow is
>> -	 * already orphaned
>> -	 */
>> -	if (ssk->sk_socket)
>> -		sock_orphan(ssk);
>> -
>> +	sock_orphan(ssk);
>>  	subflow->disposable = 1;
>>  
>>  	/* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops
>> @@ -2940,7 +2935,11 @@ bool __mptcp_close(struct sock *sk, long timeout)
>>  		if (ssk == msk->first)
>>  			subflow->fail_tout = 0;
>>  
>> -		sock_orphan(ssk);
>> +		/* detach from the parent socket, but allow data_ready to
>> +		 * push incoming date into the mptcp stack, to properly ack it
> 
> Nit:                             ^^^^
> 
> should be 'data'
> 
> @Mat: could you please fix it while applying or do you prefer a v3 or a
> squash-to patch?

Yes, no need to send a v3 ;-)

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

Re: mptcp: don't orphan ssk in mptcp_close(): Tests Results

Posted by MPTCP CI 1 year, 5 months ago

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6300542272536576
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6300542272536576/summary/summary.txt

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4893167388983296
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4893167388983296/summary/summary.txt

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


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)