MPTCP Linux Development
 help / color / mirror / Atom feed
* [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset
@ 2025-11-07  7:23 Paolo Abeni
  2025-11-07  7:23 ` [PATCH v2 mptcp-net 1/2] mptcp: decouple mptcp fastclose from tcp close Paolo Abeni
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Paolo Abeni @ 2025-11-07  7:23 UTC (permalink / raw)
  To: mptcp; +Cc: geliang

The actual fix is in patch 2/2, while patch 1/2 is there just to make
the change tidy/more easily reviewable.
---
v1 -> v2:
  - drop FASTCLOSE flag entirely

Paolo Abeni (2):
  mptcp: decouple mptcp fastclose from tcp close
  mptcp: fix duplicate reset on fastclose

 net/mptcp/protocol.c | 50 ++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 18 deletions(-)

-- 
2.51.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 mptcp-net 1/2] mptcp: decouple mptcp fastclose from tcp close
  2025-11-07  7:23 [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset Paolo Abeni
@ 2025-11-07  7:23 ` Paolo Abeni
  2025-11-11 11:24   ` Matthieu Baerts
  2025-11-07  7:23 ` [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose Paolo Abeni
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Paolo Abeni @ 2025-11-07  7:23 UTC (permalink / raw)
  To: mptcp; +Cc: geliang

With the current fastclose implementation, the mptcp_do_fastclose() helper
is in charge of two distinct actions: send the fastclose reset and cleanup
the subflows.

Formally decouple the two steps, ensuring that mptcp explicitly closes all
the subflows after the mentioned helper.

This will make the upcoming fix simpler, and allows dropping the 2nd
argument from mptcp_destroy_common().

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1506fde9a6da..0301e0b0de05 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2884,7 +2884,11 @@ static void mptcp_worker(struct work_struct *work)
 		__mptcp_close_subflow(sk);
 
 	if (mptcp_close_tout_expired(sk)) {
+		struct mptcp_subflow_context *subflow, *tmp;
+
 		mptcp_do_fastclose(sk);
+		mptcp_for_each_subflow_safe(msk, subflow, tmp)
+			__mptcp_close_ssk(sk, subflow->tcp_sock, subflow, 0);
 		mptcp_close_wake_up(sk);
 	}
 
@@ -3289,7 +3293,7 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
 	inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
 }
 
-static void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
+static void mptcp_destroy_common(struct mptcp_sock *msk)
 {
 	struct mptcp_subflow_context *subflow, *tmp;
 	struct sock *sk = (struct sock *)msk;
@@ -3299,7 +3303,7 @@ static void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
 
 	/* join list will be eventually flushed (with rst) at sock lock release time */
 	mptcp_for_each_subflow_safe(msk, subflow, tmp)
-		__mptcp_close_ssk(sk, mptcp_subflow_tcp_sock(subflow), subflow, flags);
+		__mptcp_close_ssk(sk, mptcp_subflow_tcp_sock(subflow), subflow, 0);
 
 	__skb_queue_purge(&sk->sk_receive_queue);
 	skb_rbtree_purge(&msk->out_of_order_queue);
@@ -3333,7 +3337,8 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	/* msk->subflow is still intact, the following will not free the first
 	 * subflow
 	 */
-	mptcp_destroy_common(msk, MPTCP_CF_FASTCLOSE);
+	mptcp_do_fastclose(sk);
+	mptcp_destroy_common(msk);
 
 	/* The first subflow is already in TCP_CLOSE status, the following
 	 * can't overlap with a fallback anymore
@@ -3521,7 +3526,7 @@ static void mptcp_destroy(struct sock *sk)
 
 	/* allow the following to close even the initial subflow */
 	msk->free_first = 1;
-	mptcp_destroy_common(msk, 0);
+	mptcp_destroy_common(msk);
 	sk_sockets_allocated_dec(sk);
 }
 
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose
  2025-11-07  7:23 [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset Paolo Abeni
  2025-11-07  7:23 ` [PATCH v2 mptcp-net 1/2] mptcp: decouple mptcp fastclose from tcp close Paolo Abeni
@ 2025-11-07  7:23 ` Paolo Abeni
  2025-11-11  1:59   ` Geliang Tang
  2025-11-11 11:24   ` Matthieu Baerts
  2025-11-07  8:30 ` [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset MPTCP CI
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 12+ messages in thread
From: Paolo Abeni @ 2025-11-07  7:23 UTC (permalink / raw)
  To: mptcp; +Cc: geliang

The CI reports sporadic failures of the fastclose self-tests. The root
cause is a duplicate reset, not carrying the relevant MPTCP option.
In the failing scenario the bad reset is received by the peer before
the fastclose one, preventing the reception of the latter.

Indeed there is window of opportunity at fastclose time for the following
race:

mptcp_do_fastclose
  __mptcp_close_ssk
    __tcp_close()
      tcp_set_state() [1]
      tcp_send_active_reset() [2]

After [1] the stack will send reset to in-flight data reaching the now
closed port. Such reset may race with [2].

Address the issue explicitly sending a single reset on fastclose before
explicitly moving the subflow to close status.

Fixes: d21f83485518 ("mptcp: use fastclose on more edge scenarios");
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/596
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - test subflow->send_fastclose in __mptcp_subflow_disconnect() instead
   of MPTCP_CF_FASTCLOSE
---
 net/mptcp/protocol.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0301e0b0de05..24d4fa8227b7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2437,7 +2437,6 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
 
 /* flags for __mptcp_close_ssk() */
 #define MPTCP_CF_PUSH		BIT(1)
-#define MPTCP_CF_FASTCLOSE	BIT(2)
 
 /* be sure to send a reset only if the caller asked for it, also
  * clean completely the subflow status when the subflow reaches
@@ -2448,7 +2447,7 @@ static void __mptcp_subflow_disconnect(struct sock *ssk,
 				       unsigned int flags)
 {
 	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
-	    (flags & MPTCP_CF_FASTCLOSE)) {
+	    subflow->send_fastclose) {
 		/* The MPTCP code never wait on the subflow sockets, TCP-level
 		 * disconnect should never fail
 		 */
@@ -2511,20 +2510,13 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	if (dispose_it)
 		list_del(&subflow->node);
 
-	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
-		/* be sure to force the tcp_close path
-		 * to generate the egress reset
-		 */
-		ssk->sk_lingertime = 0;
-		sock_set_flag(ssk, SOCK_LINGER);
-		subflow->send_fastclose = 1;
-	}
+	if (subflow->send_fastclose && ssk->sk_state != TCP_CLOSE)
+		tcp_set_state(ssk, TCP_CLOSE);
 
 	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
 	if (!dispose_it) {
 		__mptcp_subflow_disconnect(ssk, subflow, flags);
 		release_sock(ssk);
-
 		goto out;
 	}
 
@@ -2855,9 +2847,26 @@ static void mptcp_do_fastclose(struct sock *sk)
 
 	mptcp_set_state(sk, TCP_CLOSE);
 	mptcp_backlog_purge(sk);
-	mptcp_for_each_subflow_safe(msk, subflow, tmp)
-		__mptcp_close_ssk(sk, mptcp_subflow_tcp_sock(subflow),
-				  subflow, MPTCP_CF_FASTCLOSE);
+
+	/* Explicitly send the fastclose reset as need */
+	if (__mptcp_check_fallback(msk))
+		return;
+
+	mptcp_for_each_subflow_safe(msk, subflow, tmp) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+		lock_sock(ssk);
+
+		/* Some subflow socket states don't allow/need a reset.*/
+		if ((1 << ssk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))
+			goto unlock;
+
+		subflow->send_fastclose = 1;
+		tcp_send_active_reset(ssk, ssk->sk_allocation,
+				      SK_RST_REASON_TCP_ABORT_ON_CLOSE);
+unlock:
+		release_sock(ssk);
+	}
 }
 
 static void mptcp_worker(struct work_struct *work)
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset
  2025-11-07  7:23 [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset Paolo Abeni
  2025-11-07  7:23 ` [PATCH v2 mptcp-net 1/2] mptcp: decouple mptcp fastclose from tcp close Paolo Abeni
  2025-11-07  7:23 ` [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose Paolo Abeni
@ 2025-11-07  8:30 ` MPTCP CI
  2025-11-11  2:31 ` Geliang Tang
  2025-11-11 15:54 ` Matthieu Baerts
  4 siblings, 0 replies; 12+ messages in thread
From: MPTCP CI @ 2025-11-07  8:30 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Unstable: 1 failed test(s): packetdrill_mp_capable 🔴
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/19161574021

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/92f738c5d9cf
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1020769


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-normal

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 (NGI0 Core)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose
  2025-11-07  7:23 ` [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose Paolo Abeni
@ 2025-11-11  1:59   ` Geliang Tang
  2025-11-11  6:14     ` Matthieu Baerts
  2025-11-11 11:24   ` Matthieu Baerts
  1 sibling, 1 reply; 12+ messages in thread
From: Geliang Tang @ 2025-11-11  1:59 UTC (permalink / raw)
  To: Paolo Abeni, mptcp

Hi Paolo,

Thanks for this v2.

On Fri, 2025-11-07 at 08:23 +0100, Paolo Abeni wrote:
> The CI reports sporadic failures of the fastclose self-tests. The
> root
> cause is a duplicate reset, not carrying the relevant MPTCP option.
> In the failing scenario the bad reset is received by the peer before
> the fastclose one, preventing the reception of the latter.
> 
> Indeed there is window of opportunity at fastclose time for the
> following
> race:
> 
> mptcp_do_fastclose
>   __mptcp_close_ssk
>     __tcp_close()
>       tcp_set_state() [1]
>       tcp_send_active_reset() [2]
> 
> After [1] the stack will send reset to in-flight data reaching the
> now
> closed port. Such reset may race with [2].
> 
> Address the issue explicitly sending a single reset on fastclose
> before
> explicitly moving the subflow to close status.
> 
> Fixes: d21f83485518 ("mptcp: use fastclose on more edge scenarios");
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/596
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>  - test subflow->send_fastclose in __mptcp_subflow_disconnect()
> instead
>    of MPTCP_CF_FASTCLOSE
> ---
>  net/mptcp/protocol.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0301e0b0de05..24d4fa8227b7 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2437,7 +2437,6 @@ bool __mptcp_retransmit_pending_data(struct
> sock *sk)
>  
>  /* flags for __mptcp_close_ssk() */
>  #define MPTCP_CF_PUSH		BIT(1)
> -#define MPTCP_CF_FASTCLOSE	BIT(2)
>  
>  /* be sure to send a reset only if the caller asked for it, also
>   * clean completely the subflow status when the subflow reaches
> @@ -2448,7 +2447,7 @@ static void __mptcp_subflow_disconnect(struct
> sock *ssk,
>  				       unsigned int flags)

nit:

The 3rd argument "flags" of __mptcp_subflow_disconnect is useless now.
We can drop it.

No need to send v3, Matt or I can handle it.

Thanks,
-Geliang

>  {
>  	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
> -	    (flags & MPTCP_CF_FASTCLOSE)) {
> +	    subflow->send_fastclose) {
>  		/* The MPTCP code never wait on the subflow sockets,
> TCP-level
>  		 * disconnect should never fail
>  		 */
> @@ -2511,20 +2510,13 @@ static void __mptcp_close_ssk(struct sock
> *sk, struct sock *ssk,
>  	if (dispose_it)
>  		list_del(&subflow->node);
>  
> -	if ((flags & MPTCP_CF_FASTCLOSE) &&
> !__mptcp_check_fallback(msk)) {
> -		/* be sure to force the tcp_close path
> -		 * to generate the egress reset
> -		 */
> -		ssk->sk_lingertime = 0;
> -		sock_set_flag(ssk, SOCK_LINGER);
> -		subflow->send_fastclose = 1;
> -	}
> +	if (subflow->send_fastclose && ssk->sk_state != TCP_CLOSE)
> +		tcp_set_state(ssk, TCP_CLOSE);
>  
>  	need_push = (flags & MPTCP_CF_PUSH) &&
> __mptcp_retransmit_pending_data(sk);
>  	if (!dispose_it) {
>  		__mptcp_subflow_disconnect(ssk, subflow, flags);
>  		release_sock(ssk);
> -
>  		goto out;
>  	}
>  
> @@ -2855,9 +2847,26 @@ static void mptcp_do_fastclose(struct sock
> *sk)
>  
>  	mptcp_set_state(sk, TCP_CLOSE);
>  	mptcp_backlog_purge(sk);
> -	mptcp_for_each_subflow_safe(msk, subflow, tmp)
> -		__mptcp_close_ssk(sk,
> mptcp_subflow_tcp_sock(subflow),
> -				  subflow, MPTCP_CF_FASTCLOSE);
> +
> +	/* Explicitly send the fastclose reset as need */
> +	if (__mptcp_check_fallback(msk))
> +		return;
> +
> +	mptcp_for_each_subflow_safe(msk, subflow, tmp) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		lock_sock(ssk);
> +
> +		/* Some subflow socket states don't allow/need a
> reset.*/
> +		if ((1 << ssk->sk_state) & (TCPF_LISTEN |
> TCPF_CLOSE))
> +			goto unlock;
> +
> +		subflow->send_fastclose = 1;
> +		tcp_send_active_reset(ssk, ssk->sk_allocation,
> +				     
> SK_RST_REASON_TCP_ABORT_ON_CLOSE);
> +unlock:
> +		release_sock(ssk);
> +	}
>  }
>  
>  static void mptcp_worker(struct work_struct *work)


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset
  2025-11-07  7:23 [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset Paolo Abeni
                   ` (2 preceding siblings ...)
  2025-11-07  8:30 ` [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset MPTCP CI
@ 2025-11-11  2:31 ` Geliang Tang
  2025-11-11 15:54 ` Matthieu Baerts
  4 siblings, 0 replies; 12+ messages in thread
From: Geliang Tang @ 2025-11-11  2:31 UTC (permalink / raw)
  To: Paolo Abeni, mptcp

Hi Paolo,

On Fri, 2025-11-07 at 08:23 +0100, Paolo Abeni wrote:
> The actual fix is in patch 2/2, while patch 1/2 is there just to make
> the change tidy/more easily reviewable.
> ---
> v1 -> v2:
>   - drop FASTCLOSE flag entirely

Apart from a nit in patch 2, this set looks good to me.

    Reviewed-by: Geliang Tang <geliang@kernel.org>

Note: As discussed in v1, with this set, "selftests: mptcp: join:
fastclose: drop plain RST" should be reverted.

Thanks,
-Geliang

> 
> Paolo Abeni (2):
>   mptcp: decouple mptcp fastclose from tcp close
>   mptcp: fix duplicate reset on fastclose
> 
>  net/mptcp/protocol.c | 50 ++++++++++++++++++++++++++++--------------
> --
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose
  2025-11-11  1:59   ` Geliang Tang
@ 2025-11-11  6:14     ` Matthieu Baerts
  2025-11-11  7:27       ` Paolo Abeni
  0 siblings, 1 reply; 12+ messages in thread
From: Matthieu Baerts @ 2025-11-11  6:14 UTC (permalink / raw)
  To: Geliang Tang; +Cc: Paolo Abeni, mptcp

Hi Geliang,

11 Nov 2025 02:59:28 Geliang Tang <geliang@kernel.org>:

> Hi Paolo,
>
> Thanks for this v2.
>
> On Fri, 2025-11-07 at 08:23 +0100, Paolo Abeni wrote:
>> The CI reports sporadic failures of the fastclose self-tests. The
>> root
>> cause is a duplicate reset, not carrying the relevant MPTCP option.
>> In the failing scenario the bad reset is received by the peer before
>> the fastclose one, preventing the reception of the latter.
>>
>> Indeed there is window of opportunity at fastclose time for the
>> following
>> race:
>>
>> mptcp_do_fastclose
>>   __mptcp_close_ssk
>>     __tcp_close()
>>       tcp_set_state() [1]
>>       tcp_send_active_reset() [2]
>>
>> After [1] the stack will send reset to in-flight data reaching the
>> now
>> closed port. Such reset may race with [2].
>>
>> Address the issue explicitly sending a single reset on fastclose
>> before
>> explicitly moving the subflow to close status.
>>
>> Fixes: d21f83485518 ("mptcp: use fastclose on more edge scenarios");
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/596
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> v1 -> v2:
>>  - test subflow->send_fastclose in __mptcp_subflow_disconnect()
>> instead
>>    of MPTCP_CF_FASTCLOSE
>> ---
>>  net/mptcp/protocol.c | 37 +++++++++++++++++++++++--------------
>>  1 file changed, 23 insertions(+), 14 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 0301e0b0de05..24d4fa8227b7 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -2437,7 +2437,6 @@ bool __mptcp_retransmit_pending_data(struct
>> sock *sk)
>>  
>>  /* flags for __mptcp_close_ssk() */
>>  #define MPTCP_CF_PUSH      BIT(1)
>> -#define MPTCP_CF_FASTCLOSE BIT(2)
>>  
>>  /* be sure to send a reset only if the caller asked for it, also
>>   * clean completely the subflow status when the subflow reaches
>> @@ -2448,7 +2447,7 @@ static void __mptcp_subflow_disconnect(struct
>> sock *ssk,
>>                        unsigned int flags)
>
> nit:
>
> The 3rd argument "flags" of __mptcp_subflow_disconnect is useless now.
> We can drop it.

I don't think it is useless: __mptcp_close_ssk() is still called with either
the push flag or no flag (0).

Because this patch is for net, we should avoid unnecessary modifications:
we should not change the type or the name of the function argument for
"cosmetic" reasons.

>
> No need to send v3, Matt or I can handle it.
>
> Thanks,
> -Geliang
>
>>  {
>>     if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
>> -       (flags & MPTCP_CF_FASTCLOSE)) {
>> +       subflow->send_fastclose) {
>>         /* The MPTCP code never wait on the subflow sockets,
>> TCP-level
>>          * disconnect should never fail
>>          */
>> @@ -2511,20 +2510,13 @@ static void __mptcp_close_ssk(struct sock
>> *sk, struct sock *ssk,
>>     if (dispose_it)
>>         list_del(&subflow->node);
>>  
>> -   if ((flags & MPTCP_CF_FASTCLOSE) &&
>> !__mptcp_check_fallback(msk)) {
>> -       /* be sure to force the tcp_close path
>> -        * to generate the egress reset
>> -        */
>> -       ssk->sk_lingertime = 0;
>> -       sock_set_flag(ssk, SOCK_LINGER);
>> -       subflow->send_fastclose = 1;
>> -   }
>> +   if (subflow->send_fastclose && ssk->sk_state != TCP_CLOSE)
>> +       tcp_set_state(ssk, TCP_CLOSE);
>>  
>>     need_push = (flags & MPTCP_CF_PUSH) &&
>> __mptcp_retransmit_pending_data(sk);
>>     if (!dispose_it) {
>>         __mptcp_subflow_disconnect(ssk, subflow, flags);
>>         release_sock(ssk);
>> -

(This line should not be removed but this can be fixed when applying the patch.)

Cheers,
Matt

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose
  2025-11-11  6:14     ` Matthieu Baerts
@ 2025-11-11  7:27       ` Paolo Abeni
  2025-11-11  7:49         ` Geliang Tang
  0 siblings, 1 reply; 12+ messages in thread
From: Paolo Abeni @ 2025-11-11  7:27 UTC (permalink / raw)
  To: Matthieu Baerts, Geliang Tang; +Cc: mptcp

On 11/11/25 7:14 AM, Matthieu Baerts wrote:
> 11 Nov 2025 02:59:28 Geliang Tang <geliang@kernel.org>:
>> On Fri, 2025-11-07 at 08:23 +0100, Paolo Abeni wrote:
>>> @@ -2448,7 +2447,7 @@ static void __mptcp_subflow_disconnect(struct
>>> sock *ssk,
>>>                        unsigned int flags)
>>
>> nit:
>>
>> The 3rd argument "flags" of __mptcp_subflow_disconnect is useless now.
>> We can drop it.
> 
> I don't think it is useless: __mptcp_close_ssk() is still called with either
> the push flag or no flag (0).
> 
> Because this patch is for net, we should avoid unnecessary modifications:
> we should not change the type or the name of the function argument for
> "cosmetic" reasons.

FWIW, I agree with Mat.

>>> @@ -2511,20 +2510,13 @@ static void __mptcp_close_ssk(struct sock
>>> *sk, struct sock *ssk,
>>>     if (dispose_it)
>>>         list_del(&subflow->node);
>>>  
>>> -   if ((flags & MPTCP_CF_FASTCLOSE) &&
>>> !__mptcp_check_fallback(msk)) {
>>> -       /* be sure to force the tcp_close path
>>> -        * to generate the egress reset
>>> -        */
>>> -       ssk->sk_lingertime = 0;
>>> -       sock_set_flag(ssk, SOCK_LINGER);
>>> -       subflow->send_fastclose = 1;
>>> -   }
>>> +   if (subflow->send_fastclose && ssk->sk_state != TCP_CLOSE)
>>> +       tcp_set_state(ssk, TCP_CLOSE);
>>>  
>>>     need_push = (flags & MPTCP_CF_PUSH) &&
>>> __mptcp_retransmit_pending_data(sk);
>>>     if (!dispose_it) {
>>>         __mptcp_subflow_disconnect(ssk, subflow, flags);
>>>         release_sock(ssk);
>>> -
> 
> (This line should not be removed but this can be fixed when applying the patch.)

Thanks!

/P


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose
  2025-11-11  7:27       ` Paolo Abeni
@ 2025-11-11  7:49         ` Geliang Tang
  0 siblings, 0 replies; 12+ messages in thread
From: Geliang Tang @ 2025-11-11  7:49 UTC (permalink / raw)
  To: Paolo Abeni, Matthieu Baerts; +Cc: mptcp

On Tue, 2025-11-11 at 08:27 +0100, Paolo Abeni wrote:
> On 11/11/25 7:14 AM, Matthieu Baerts wrote:
> > 11 Nov 2025 02:59:28 Geliang Tang <geliang@kernel.org>:
> > > On Fri, 2025-11-07 at 08:23 +0100, Paolo Abeni wrote:
> > > > @@ -2448,7 +2447,7 @@ static void
> > > > __mptcp_subflow_disconnect(struct
> > > > sock *ssk,
> > > >                        unsigned int flags)
> > > 
> > > nit:
> > > 
> > > The 3rd argument "flags" of __mptcp_subflow_disconnect is useless
> > > now.
> > > We can drop it.
> > 
> > I don't think it is useless: __mptcp_close_ssk() is still called
> > with either
> > the push flag or no flag (0).
> > 
> > Because this patch is for net, we should avoid unnecessary
> > modifications:
> > we should not change the type or the name of the function argument
> > for
> > "cosmetic" reasons.
> 
> FWIW, I agree with Mat.

I agree too. Let's apply it as is.

Thanks,
-Geliang

> 
> > > > @@ -2511,20 +2510,13 @@ static void __mptcp_close_ssk(struct
> > > > sock
> > > > *sk, struct sock *ssk,
> > > >     if (dispose_it)
> > > >         list_del(&subflow->node);
> > > >  
> > > > -   if ((flags & MPTCP_CF_FASTCLOSE) &&
> > > > !__mptcp_check_fallback(msk)) {
> > > > -       /* be sure to force the tcp_close path
> > > > -        * to generate the egress reset
> > > > -        */
> > > > -       ssk->sk_lingertime = 0;
> > > > -       sock_set_flag(ssk, SOCK_LINGER);
> > > > -       subflow->send_fastclose = 1;
> > > > -   }
> > > > +   if (subflow->send_fastclose && ssk->sk_state != TCP_CLOSE)
> > > > +       tcp_set_state(ssk, TCP_CLOSE);
> > > >  
> > > >     need_push = (flags & MPTCP_CF_PUSH) &&
> > > > __mptcp_retransmit_pending_data(sk);
> > > >     if (!dispose_it) {
> > > >         __mptcp_subflow_disconnect(ssk, subflow, flags);
> > > >         release_sock(ssk);
> > > > -
> > 
> > (This line should not be removed but this can be fixed when
> > applying the patch.)
> 
> Thanks!
> 
> /P
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 1/2] mptcp: decouple mptcp fastclose from tcp close
  2025-11-07  7:23 ` [PATCH v2 mptcp-net 1/2] mptcp: decouple mptcp fastclose from tcp close Paolo Abeni
@ 2025-11-11 11:24   ` Matthieu Baerts
  0 siblings, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2025-11-11 11:24 UTC (permalink / raw)
  To: Paolo Abeni, mptcp; +Cc: geliang

Hi Paolo,

On 07/11/2025 08:23, Paolo Abeni wrote:
> With the current fastclose implementation, the mptcp_do_fastclose() helper
> is in charge of two distinct actions: send the fastclose reset and cleanup
> the subflows.
> 
> Formally decouple the two steps, ensuring that mptcp explicitly closes all
> the subflows after the mentioned helper.
> 
> This will make the upcoming fix simpler, and allows dropping the 2nd
> argument from mptcp_destroy_common().

I suggest adding the same Fixes tag here, just to ease the backports (to
ensure this patch is automatically picked up):

Fixes: d21f83485518 ("mptcp: use fastclose on more edge scenarios")

I'm going to add the following text to the previous paragraph:

  The Fixes tag is the same as in the next commit, simply to ease the
  backports.

Thank you for the patch!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose
  2025-11-07  7:23 ` [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose Paolo Abeni
  2025-11-11  1:59   ` Geliang Tang
@ 2025-11-11 11:24   ` Matthieu Baerts
  1 sibling, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2025-11-11 11:24 UTC (permalink / raw)
  To: Paolo Abeni, mptcp; +Cc: geliang

Hi Paolo,

On 07/11/2025 08:23, Paolo Abeni wrote:
> The CI reports sporadic failures of the fastclose self-tests. The root
> cause is a duplicate reset, not carrying the relevant MPTCP option.
> In the failing scenario the bad reset is received by the peer before
> the fastclose one, preventing the reception of the latter.
> 
> Indeed there is window of opportunity at fastclose time for the following
> race:
> 
> mptcp_do_fastclose
>   __mptcp_close_ssk
>     __tcp_close()
>       tcp_set_state() [1]
>       tcp_send_active_reset() [2]
> 
> After [1] the stack will send reset to in-flight data reaching the now
> closed port. Such reset may race with [2].
> 
> Address the issue explicitly sending a single reset on fastclose before
> explicitly moving the subflow to close status.

Thank you for the proper fix!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Fixes: d21f83485518 ("mptcp: use fastclose on more edge scenarios");

When applying this patch, I will remove the extra ';' at the end of this
line, plus not removing a line below.

I will also revert "selftests: mptcp: join: fastclose: drop plain RST"
and only keep the removal of the two 'MPTCP_LIB_SUBTEST_FLAKY=1' lines.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset
  2025-11-07  7:23 [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset Paolo Abeni
                   ` (3 preceding siblings ...)
  2025-11-11  2:31 ` Geliang Tang
@ 2025-11-11 15:54 ` Matthieu Baerts
  4 siblings, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2025-11-11 15:54 UTC (permalink / raw)
  To: Paolo Abeni, mptcp; +Cc: geliang

Hi Paolo, Geliang,

(I just noticed this email was unsent, the patches have been applied a
few hours ago)

On 07/11/2025 08:23, Paolo Abeni wrote:
> The actual fix is in patch 2/2, while patch 1/2 is there just to make
> the change tidy/more easily reviewable.

Thank you for the patches and the reviews! I had a few conflicts, but I
think I got it right.

Now in our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- 408b267c6bd8: Revert "selftests: mptcp: join: fastclose: drop plain RST"
- 0ef8d2d40280: mptcp: decouple mptcp fastclose from tcp close (with
conflicts)
- 5413ee2c5a4b: mptcp: fix duplicate reset on fastclose (with conflicts)
- a5fd6152dbe2: conflict in t/mptcp-make-mptcp_destroy_common-static
- 69ec39f082d0: conflict in
t/mptcp-handle-first-subflow-closing-consistently
- 8a23d7a4757f: conflict in t/mptcp-introduce-mptcp-level-backlog
- e54a7311c74b: selftests: mptcp: join: fastclose: remove flaky marks
- Results: 0a46d88dd962..751a2432b7d8 (export-net)
- Results: 3ba6375d918e..247540d77d20 (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/7b747795627aab975ffff54ee796ccd77f593292/checks
- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/3f20649f33461a575a29a4da06d29ec9c50058cb/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/cf56fe8fbd0dde4dcb53f37ba20d59ac543bbf74/checks

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2025-11-11 15:54 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-07  7:23 [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset Paolo Abeni
2025-11-07  7:23 ` [PATCH v2 mptcp-net 1/2] mptcp: decouple mptcp fastclose from tcp close Paolo Abeni
2025-11-11 11:24   ` Matthieu Baerts
2025-11-07  7:23 ` [PATCH v2 mptcp-net 2/2] mptcp: fix duplicate reset on fastclose Paolo Abeni
2025-11-11  1:59   ` Geliang Tang
2025-11-11  6:14     ` Matthieu Baerts
2025-11-11  7:27       ` Paolo Abeni
2025-11-11  7:49         ` Geliang Tang
2025-11-11 11:24   ` Matthieu Baerts
2025-11-07  8:30 ` [PATCH v2 mptcp-net 0/2] mptcp: fix duplicate reset MPTCP CI
2025-11-11  2:31 ` Geliang Tang
2025-11-11 15:54 ` Matthieu Baerts

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox