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

[sashiko-bot@kernel.org]

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