[PATCH mptcp-net 1/2] mptcp: prevent race between disconnect() and rtx

[PATCH mptcp-net 1/2] mptcp: prevent race between disconnect() and rtx

[Paolo Abeni]

Sashiko noted that the two event can race, leading to inconsistent
status. Prevent the race using the synchronous timer stop operation.

Fixes: b29fcfb54cd7 ("mptcp: full disconnect implementation")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f22d64ab1c53..1e7e59d497c5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3625,6 +3625,7 @@ static void mptcp_destroy_common(struct mptcp_sock *msk)
 
 static int mptcp_disconnect(struct sock *sk, int flags)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct mptcp_sock *msk = mptcp_sk(sk);
 
 	/* We are on the fastopen error path. We can't call straight into the
@@ -3637,8 +3638,13 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	mptcp_check_listen_stop(sk);
 	mptcp_set_state(sk, TCP_CLOSE);
 
-	mptcp_stop_rtx_timer(sk);
-	mptcp_stop_tout_timer(sk);
+	/* The later subflow close can not kick again the tout timer,
+	 * as the msk is already in closed status.
+	 */
+	msk->timer_ival = icsk->icsk_rto_min;
+	sk_stop_timer_sync(sk, &sk->mptcp_retransmit_timer);
+	icsk->icsk_mtup.probe_timestamp = 0;
+	sk_stop_timer_sync(sk, &icsk->mptcp_tout_timer);
 
 	mptcp_pm_connection_closed(msk);
 
-- 
2.55.0

[PATCH mptcp-net 2/2] Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

[Paolo Abeni]

Sashiko noted that the 'RTX disabled' status is carried over
across connect() failures, potentially to subsequent successful connect()
or listen().

Explicitly control the RTX enabling status across the whole msk life-cycle.
To make the code more straight forward switch the newly introduced flag
semantic.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 8 +++++---
 net/mptcp/protocol.h | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1e7e59d497c5..db745d9dde59 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -96,7 +96,7 @@ bool __mptcp_try_fallback(struct mptcp_sock *msk, int fb_mib)
 
 	msk->allow_subflows = false;
 	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
-	set_bit(MPTCP_RTX_DISABLED, &msk->flags);
+	clear_bit(MPTCP_RTX_ENABLED, &msk->flags);
 	__MPTCP_INC_STATS(net, fb_mib);
 	spin_unlock_bh(&msk->fallback_lock);
 	return true;
@@ -1126,7 +1126,7 @@ static void mptcp_reset_rtx_timer(struct sock *sk)
 	unsigned long tout;
 
 	/* Prevent rescheduling on close and in case of fallback. */
-	if (test_bit(MPTCP_RTX_DISABLED, &msk->flags))
+	if (!test_bit(MPTCP_RTX_ENABLED, &msk->flags))
 		return;
 
 	tout = msk->timer_ival;
@@ -3363,7 +3363,7 @@ void mptcp_set_state(struct sock *sk, int state)
 		 */
 		break;
 	case TCP_CLOSE:
-		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
+		clear_bit(MPTCP_RTX_ENABLED, &mptcp_sk(sk)->flags);
 		fallthrough;
 	default:
 		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
@@ -3785,6 +3785,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 	/* passive msk is created after the first/MPC subflow */
 	msk->subflow_id = 2;
 
+	set_bit(MPTCP_RTX_ENABLED, &msk->flags);
 	sock_reset_flag(nsk, SOCK_RCU_FREE);
 	security_inet_csk_clone(nsk, req);
 
@@ -4190,6 +4191,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
 		return PTR_ERR(ssk);
 
 	mptcp_set_state(sk, TCP_SYN_SENT);
+	set_bit(MPTCP_RTX_ENABLED, &msk->flags);
 	subflow = mptcp_subflow_ctx(ssk);
 #ifdef CONFIG_TCP_MD5SIG
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3d250e8204d5..482d4a549881 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -116,7 +116,7 @@
 #define MPTCP_WORK_RTX		1
 #define MPTCP_FALLBACK_DONE	2
 #define MPTCP_WORK_CLOSE_SUBFLOW 3
-#define MPTCP_RTX_DISABLED	4
+#define MPTCP_RTX_ENABLED	4
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1
-- 
2.55.0

Re: [PATCH mptcp-net 2/2] Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Non-blocking MPTCP Fastopen bypasses `mptcp_connect`, leaving the socket permanently in `TCP_CLOSE` and failing to enable the RTX timer.
--

commit 4679eada053d3ad7385facbcb9d12c98ccd146f2
Author: Paolo Abeni <pabeni@redhat.com>
Subject: Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

This commit replaces the MPTCP_RTX_DISABLED flag with MPTCP_RTX_ENABLED to
explicitly control the retransmit timer status across the socket lifecycle.
It sets the enabled flag during active connect and passive initialization.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2f17c5daa704e..a28a0506d28da 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -4154,6 +4155,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
>  		return PTR_ERR(ssk);
>  
>  	mptcp_set_state(sk, TCP_SYN_SENT);
> +	set_bit(MPTCP_RTX_ENABLED, &msk->flags);

[Severity: High]
Does this change leave the RTX timer disabled during non-blocking Fastopen
connections?

When a non-blocking TCP Fast Open connection is initiated on an MPTCP socket
(using MSG_FASTOPEN and MSG_DONTWAIT), mptcp_sendmsg_fastopen() skips the call
to __inet_stream_connect(), which bypasses mptcp_connect() entirely:

net/mptcp/protocol.c:mptcp_sendmsg_fastopen() {
    ...
	if (ret == -EINPROGRESS && !(msg->msg_flags & MSG_DONTWAIT)) {
		ret = __inet_stream_connect(sk->sk_socket, msg->msg_name,
					    msg->msg_namelen, msg->msg_flags, 1);
    ...
}

Because mptcp_connect() is bypassed, MPTCP_RTX_ENABLED is never set, and the
socket state remains TCP_CLOSE instead of TCP_SYN_SENT.

This causes __mptcp_sync_state() to skip transitioning the socket to
TCP_ESTABLISHED:

net/mptcp/subflow.c:__mptcp_sync_state() {
    ...
	if (sk->sk_state == TCP_SYN_SENT) {
		/* subflow->idsn is always available is TCP_SYN_SENT state,
		 * even for the FASTOPEN scenarios
		 */
    ...
}

Will this cause non-blocking Fastopen connections to become permanently stuck
in TCP_CLOSE without an active retransmit timer if packet loss occurs?

>  	subflow = mptcp_subflow_ctx(ssk);
>  #ifdef CONFIG_TCP_MD5SIG
>  	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f1e8ffe44c3756195391a25ecf60e56e02e0f34f.1787739122.git.pabeni@redhat.com?part=2

Re: [PATCH mptcp-net 2/2] Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

[Paolo Abeni]

On 8/26/26 12:30 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Non-blocking MPTCP Fastopen bypasses `mptcp_connect`, leaving the socket permanently in `TCP_CLOSE` and failing to enable the RTX timer.
> --
> 
> commit 4679eada053d3ad7385facbcb9d12c98ccd146f2
> Author: Paolo Abeni <pabeni@redhat.com>
> Subject: Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"
> 
> This commit replaces the MPTCP_RTX_DISABLED flag with MPTCP_RTX_ENABLED to
> explicitly control the retransmit timer status across the socket lifecycle.
> It sets the enabled flag during active connect and passive initialization.
> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 2f17c5daa704e..a28a0506d28da 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
> 
> [ ... ]
> 
>> @@ -4154,6 +4155,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
>>  		return PTR_ERR(ssk);
>>  
>>  	mptcp_set_state(sk, TCP_SYN_SENT);
>> +	set_bit(MPTCP_RTX_ENABLED, &msk->flags);
> 
> [Severity: High]
> Does this change leave the RTX timer disabled during non-blocking Fastopen
> connections?

Funny, opus-4.8 locally had the same concern and discarded it as
hallucination. Unfortunately I don't have the log handy.

> 
> When a non-blocking TCP Fast Open connection is initiated on an MPTCP socket
> (using MSG_FASTOPEN and MSG_DONTWAIT), mptcp_sendmsg_fastopen() skips the call
> to __inet_stream_connect(), which bypasses mptcp_connect() entirely:

It looks real, I'll send a v2.

/P

Re: [PATCH mptcp-net 2/2] Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

[MPTCP CI]

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): Success! ✅
- 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/32959977232

Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/f617456d7bb0
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1151973


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)

Re: [PATCH mptcp-net 2/2] Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

[Matthieu Baerts]

Hi Paolo,

On 26/08/2026 12:13, Paolo Abeni wrote:
> Sashiko noted that the 'RTX disabled' status is carried over
> across connect() failures, potentially to subsequent successful connect()
> or listen().
> 
> Explicitly control the RTX enabling status across the whole msk life-cycle.
> To make the code more straight forward switch the newly introduced flag
> semantic.

Thank you for looking at this!

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/protocol.c | 8 +++++---
>  net/mptcp/protocol.h | 2 +-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 1e7e59d497c5..db745d9dde59 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

(...)

> @@ -3363,7 +3363,7 @@ void mptcp_set_state(struct sock *sk, int state)
>  		 */
>  		break;
>  	case TCP_CLOSE:
> -		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
> +		clear_bit(MPTCP_RTX_ENABLED, &mptcp_sk(sk)->flags);

Could this be moved to mptcp_disconnect()? Or we prefer not, to stop
ASAP for some cases? (but I guess most of these cases switch to close
before transitioning to ESTABLISHED, no?)

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

Re: [PATCH mptcp-net 2/2] Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

[Paolo Abeni]

On 8/26/26 2:38 PM, Matthieu Baerts wrote:
> On 26/08/2026 12:13, Paolo Abeni wrote:
>> Sashiko noted that the 'RTX disabled' status is carried over
>> across connect() failures, potentially to subsequent successful connect()
>> or listen().
>>
>> Explicitly control the RTX enabling status across the whole msk life-cycle.
>> To make the code more straight forward switch the newly introduced flag
>> semantic.
> 
> Thank you for looking at this!
> 
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>>  net/mptcp/protocol.c | 8 +++++---
>>  net/mptcp/protocol.h | 2 +-
>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 1e7e59d497c5..db745d9dde59 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
> 
> (...)
> 
>> @@ -3363,7 +3363,7 @@ void mptcp_set_state(struct sock *sk, int state)
>>  		 */
>>  		break;
>>  	case TCP_CLOSE:
>> -		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
>> +		clear_bit(MPTCP_RTX_ENABLED, &mptcp_sk(sk)->flags);
> 
> Could this be moved to mptcp_disconnect()? Or we prefer not, to stop
> ASAP for some cases? (but I guess most of these cases switch to close
> before transitioning to ESTABLISHED, no?)
Why do you want to move the above to mptcp_disconnect()? It's better to
prevent the rtx timer from taking action after the msk moved to close
for any reason. IIRC we had a few syzkaller splat in the past without
such check.

/P

Re: [PATCH mptcp-net 2/2] Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

[Matthieu Baerts]

On 26/08/2026 16:38, Paolo Abeni wrote:
> On 8/26/26 2:38 PM, Matthieu Baerts wrote:
>> On 26/08/2026 12:13, Paolo Abeni wrote:
>>> Sashiko noted that the 'RTX disabled' status is carried over
>>> across connect() failures, potentially to subsequent successful connect()
>>> or listen().
>>>
>>> Explicitly control the RTX enabling status across the whole msk life-cycle.
>>> To make the code more straight forward switch the newly introduced flag
>>> semantic.
>>
>> Thank you for looking at this!
>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>>  net/mptcp/protocol.c | 8 +++++---
>>>  net/mptcp/protocol.h | 2 +-
>>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 1e7e59d497c5..db745d9dde59 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>
>> (...)
>>
>>> @@ -3363,7 +3363,7 @@ void mptcp_set_state(struct sock *sk, int state)
>>>  		 */
>>>  		break;
>>>  	case TCP_CLOSE:
>>> -		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
>>> +		clear_bit(MPTCP_RTX_ENABLED, &mptcp_sk(sk)->flags);
>>
>> Could this be moved to mptcp_disconnect()? Or we prefer not, to stop
>> ASAP for some cases? (but I guess most of these cases switch to close
>> before transitioning to ESTABLISHED, no?)
> Why do you want to move the above to mptcp_disconnect()? It's better to
> prevent the rtx timer from taking action after the msk moved to close
> for any reason. IIRC we had a few syzkaller splat in the past without
> such check.

I was wondering if it could go with the other variables that are reset
there, but I also understand we want this bit set ASAP/in all cases when
switching to CLOSE. So this flag is particular, and it makes sense to
keep it there. Nothing to modify for this.

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