Re: [PATCH resend mptcp-net 4/4] mptcp: do not reschedule the RTX timer for fallback sockets

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The MPTCP_RTX_DISABLED flag makes the TCP_CLOSE state sticky for the RTX timer, permanently disabling retransmissions on reused connections and their accepted children.
--

--- Patch [4]: [PATCH resend mptcp-net 4/4] mptcp: do not reschedule the RTX timer for fallback sockets ---
commit 227375cb381dd3864273cd7170dd28e0f079ff8b
Author: Paolo Abeni <[email protected]>

mptcp: do not reschedule the RTX timer for fallback sockets
    
This patch introduces the MPTCP_RTX_DISABLED flag to prevent the RTX timer
from being rescheduled on fallback sockets and when the socket state is
TCP_CLOSE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b311fe30c7855..f22d64ab1c53b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -1121,13 +1122,14 @@ static bool mptcp_rtx_timer_pending(struct sock *sk)
>  
>  static void mptcp_reset_rtx_timer(struct sock *sk)
>  {
> +	struct mptcp_sock *msk = mptcp_sk(sk);
>  	unsigned long tout;
>  
> -	/* prevent rescheduling on close */
> -	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
> +	/* Prevent rescheduling on close and in case of fallback. */
> +	if (test_bit(MPTCP_RTX_DISABLED, &msk->flags))
>  		return;

[Severity: High]
Since this flag is checked here to prevent rescheduling, will the timer be
permanently disabled if the flag is never cleared?

[ ... ]
> @@ -3360,6 +3362,9 @@ void mptcp_set_state(struct sock *sk, int state)
>  		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
>  		 */
>  		break;
> +	case TCP_CLOSE:
> +		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
> +		fallthrough;
>  	default:

[Severity: High]
Does setting MPTCP_RTX_DISABLED here without a corresponding clear logic make
the TCP_CLOSE state sticky for the RTX timer? 

If an MPTCP socket transitions to TCP_CLOSE (like a failed connect() attempt
or an explicit disconnect()), and the application then reuses the socket to 
connect() again, it appears the flag will remain set.

Furthermore, if listen() is called on such a reused socket, would
sk_clone_lock() copy the flags to all child sockets? This could cause every
accepted connection to inherit the MPTCP_RTX_DISABLED flag, permanently
breaking MPTCP-level retransmissions for all clients.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4