[PATCH mptcp-net v2 0/2] mptcp: more cleanups

[PATCH mptcp-net v2 0/2] mptcp: more cleanups

[Paolo Abeni]

Follow-up the the recent fixes, as per sashiko nipa feedback. A new
patch and an update to an exiting one.

See individual patches changelog.

Paolo Abeni (2):
  mptcp: prevent race between disconnect() and rtx
  Squash-to: "mptcp: do not reschedule the RTX timer for fallback
    sockets"

 net/mptcp/protocol.c | 17 ++++++++++++-----
 net/mptcp/protocol.h |  2 +-
 net/mptcp/subflow.c  |  7 +++++++
 3 files changed, 20 insertions(+), 6 deletions(-)

-- 
2.55.0

[PATCH mptcp-net v2 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 v2 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>
---
v1 -> v2:
  - consolidate enable, fix missing enable for fastopen
---
 net/mptcp/protocol.c | 7 ++++---
 net/mptcp/protocol.h | 2 +-
 net/mptcp/subflow.c  | 7 +++++++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1e7e59d497c5..d662ad5edec2 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);
 
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
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 2d7ccb01d234..c776531fcd03 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -634,6 +634,13 @@ static int subflow_chk_local_id(struct sock *sk)
 	if (likely(subflow->local_id >= 0))
 		return 0;
 
+	/* Slightly unrelated, leverage this code path being called once per
+	 * connection before sending the first packet. Early fallback could
+	 * take place at mptcp_connect() time.
+	 */
+	if (!__mptcp_check_fallback(msk))
+		set_bit(MPTCP_RTX_ENABLED, &msk->flags);
+
 	err = mptcp_pm_get_local_id(msk, (struct sock_common *)sk);
 	if (err < 0)
 		return err;
-- 
2.55.0

Re: [PATCH mptcp-net v2 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] The MPTCP retransmission timer is not properly enabled for active connections or on subsequent connect() retries due to an early return in subflow_chk_local_id(), completely breaking MPTCP retransmissions.
--

commit d97031273df9e2f644d64a1baa6fef7afaac166d
Author: Paolo Abeni <pabeni@redhat.com>

Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

This patch changes the MPTCP RTX status tracking from a 'disabled' flag
to an 'enabled' flag across the msk lifecycle.

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 01db7edce18a6..ac35ffdaf37f6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -634,6 +634,13 @@ static int subflow_chk_local_id(struct sock *sk)
>  	if (likely(subflow->local_id >= 0))
>  		return 0;
>  
> +	/* Slightly unrelated, leverage this code path being called once per
> +	 * connection before sending the first packet. Early fallback could
> +	 * take place at mptcp_connect() time.
> +	 */
> +	if (!__mptcp_check_fallback(msk))
> +		set_bit(MPTCP_RTX_ENABLED, &msk->flags);
> +

[Severity: High]
Will this flag setup ever be reached for the first subflow?

Looking at active connections, __mptcp_socket_create() explicitly
initializes the first subflow with local_id = 0:

net/mptcp/protocol.c:__mptcp_socket_create() {
	...
	/* This is the first subflow, always with id 0 */
	WRITE_ONCE(subflow->local_id, 0);
	...
}

When packet transmission calls subflow_chk_local_id(), the early return for
subflow->local_id >= 0 evaluates to true (0 >= 0), bypassing the call
to set_bit() for the MPTCP_RTX_ENABLED flag.

Because the patch flips the flag semantics from 'DISABLED' to 'ENABLED', this
leaves the timer permanently bypassed in mptcp_reset_rtx_timer() since the
flag is never set for active connections.

Additionally, when mptcp_set_state() clears this flag on the first connect()
failure during TCP_CLOSE, the flag fails to be set on retry because the
subflow is reused with local_id = 0, leaving the timer disabled during
retries as well. Can this be adjusted to ensure the flag is properly set
for active connections?

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

Re: [PATCH mptcp-net v2 0/2] mptcp: more cleanups

[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): Unstable: 3 failed test(s): selftest_mptcp_connect selftest_mptcp_connect_mmap selftest_mptcp_connect_sendfile ⚠️ 
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Unstable: 2 failed test(s): selftest_mptcp_connect_mmap selftest_mptcp_connect_sendfile ⚠️ 
- 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/32989862258

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


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 v2 2/2] Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"

[Paolo Abeni]

On 8/26/26 6:25 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The MPTCP retransmission timer is not properly enabled for active connections or on subsequent connect() retries due to an early return in subflow_chk_local_id(), completely breaking MPTCP retransmissions.
> --
> 
> commit d97031273df9e2f644d64a1baa6fef7afaac166d
> Author: Paolo Abeni <pabeni@redhat.com>
> 
> Squash-to: "mptcp: do not reschedule the RTX timer for fallback sockets"
> 
> This patch changes the MPTCP RTX status tracking from a 'disabled' flag
> to an 'enabled' flag across the msk lifecycle.
> 
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 01db7edce18a6..ac35ffdaf37f6 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -634,6 +634,13 @@ static int subflow_chk_local_id(struct sock *sk)
>>  	if (likely(subflow->local_id >= 0))
>>  		return 0;
>>  
>> +	/* Slightly unrelated, leverage this code path being called once per
>> +	 * connection before sending the first packet. Early fallback could
>> +	 * take place at mptcp_connect() time.
>> +	 */
>> +	if (!__mptcp_check_fallback(msk))
>> +		set_bit(MPTCP_RTX_ENABLED, &msk->flags);
>> +
> 
> [Severity: High]
> Will this flag setup ever be reached for the first subflow?

Oh, crap! never rush things out of the door.

V3 will be coming, but not too soon.

/P