Re: [PATCH net 01/14] mptcp: do not reschedule the RTX timer for fallback sockets

[Paolo Abeni]

On 8/27/26 9:07 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> mptcp: do not reschedule the RTX timer for fallback sockets
> 
> This replaces the TCP_CLOSE state test in mptcp_reset_rtx_timer() with a
> new MPTCP_RTX_DISABLED bit in msk->flags, set both on fallback in
> __mptcp_try_fallback() and on the transition to TCP_CLOSE in
> mptcp_set_state().
> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index b474d03620a75..23dde0b7144bb 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
> 
> [ ... ]
> 
>> @@ -1084,13 +1085,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: Medium]
> The new test depends on a flag that mptcp_disconnect() clears while the
> socket is still in TCP_CLOSE, and after mptcp_stop_rtx_timer() already ran.
> The order inside mptcp_disconnect() is:
> 
> 	mptcp_set_state(sk, TCP_CLOSE);    /* sets MPTCP_RTX_DISABLED */
> 	...
> 	mptcp_stop_rtx_timer(sk);          /* non-sync, timer_ival = 0 */
> 	...
> 	WRITE_ONCE(msk->flags, 0);         /* clears MPTCP_RTX_DISABLED */
> 	msk->cb_flags = 0;
> 
> Can the rtx timer be armed again after that point?
> 
> mptcp_stop_rtx_timer() uses sk_stop_timer() and therefore the non-sync
> timer_delete(), so an already-expired mptcp_retransmit_timer() can still be
> running on another CPU. It only takes bh_lock_sock(), and its
> __set_bit(MPTCP_RETRANSMIT, &msk->cb_flags) can land after the plain
> msk->cb_flags = 0 store above.
> 
> On the release_sock() that follows, mptcp_release_cb() calls
> __mptcp_retrans(sk) with no sk_state check, and that reaches
> __mptcp_clean_una():
> 
> out:
> 	if (snd_una == msk->snd_nxt && snd_una == msk->write_seq) {
> 		if (mptcp_rtx_timer_pending(sk) && !mptcp_data_fin_enabled(msk))
> 			mptcp_stop_rtx_timer(sk);
> 	} else {
> 		mptcp_reset_rtx_timer(sk);
> 	}
> 
> mptcp_disconnect() does not reset snd_una/snd_nxt/write_seq, so for a socket
> disconnected with unacked data the else branch runs and arms the timer with
> timer_ival == 0 on a TCP_CLOSE socket whose timers were deliberately
> stopped. The previous inet_sk_state_load(sk) == TCP_CLOSE test returned
> early here.
> 
> When that immediately-expiring timer fires, mptcp_retransmit_timer() does
> test_and_set_bit(MPTCP_WORK_RTX, &msk->flags) and then calls
> mptcp_schedule_work(), which refuses to queue the worker for a closed
> socket without clearing the bit:
> 
> 	if (inet_sk_state_load(sk) == TCP_CLOSE)
> 		return false;
> 
> If the application then reuses the socket with a new connect(), does that
> leftover MPTCP_WORK_RTX bit keep mptcp_worker() from being scheduled on
> subsequent rtx expiries, until some other mptcp_schedule_work() caller lets
> the worker clear it?
> 
>>  
>> -	tout = mptcp_sk(sk)->timer_ival;
>> +	tout = msk->timer_ival;
>>  	sk_reset_timer(sk, &sk->mptcp_retransmit_timer, jiffies + tout);
>>  }
>>  
>> @@ -3324,6 +3326,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;
> 
> [Severity: High]
> This latches the bit on every transition to TCP_CLOSE, and there is no
> clear_bit(MPTCP_RTX_DISABLED) anywhere in the tree. The only site that
> resets the word is mptcp_disconnect():
> 
> 	WRITE_ONCE(msk->flags, 0);
> 
> Can this leave the bit set on a socket that afterwards carries a normal,
> non-fallback MPTCP connection?
> 
> First path, a failed connect() followed by a retry on the same fd. The
> error path of mptcp_connect() does:
> 
> 	if (unlikely(err)) {
> 		/* avoid leaving a dangling token in an unconnected socket */
> 		mptcp_token_destroy(msk);
> 		mptcp_set_state(sk, TCP_CLOSE);
> 		return err;
> 	}
> 
> For a negative return from sk->sk_prot->connect(), __inet_stream_connect()
> takes:
> 
> 		err = sk->sk_prot->connect(sk, uaddr, addr_len);
> 		if (err < 0)
> 			goto out;
> 
> so it never reaches the sock_error: label that calls ->disconnect(), and
> mptcp_disconnect() does not run. The socket stays SS_UNCONNECTED with
> sk_state TCP_CLOSE, which is exactly what __inet_stream_connect() requires
> to accept a second connect(), and __mptcp_nmpc_sk() also permits
> TCPF_CLOSE. The later mptcp_set_state(sk, TCP_SYN_SENT) and
> mptcp_set_state(sk, TCP_ESTABLISHED) do not clear the bit.
> 
> Second path, a failed listen() and inheritance by accepted sockets.
> mptcp_listen() unconditionally does:
> 
> 	err = __inet_listen_sk(ssk, backlog);
> 	release_sock(ssk);
> 	mptcp_set_state(sk, inet_sk_state_load(ssk));
> 
> which is TCP_CLOSE on failure, so the bit is latched on the listener; a
> later successful listen() only sets TCP_LISTEN. mptcp_sk_clone_init() then
> creates accepted sockets with sk_clone_lock(), which copies the whole
> struct mptcp_sock, msk->flags included, and __mptcp_init_sock() re-inits
> the lists, timers and allow_* fields but not msk->flags.
> 
> With the bit stuck, mptcp_reset_rtx_timer() is a permanent no-op.
> MPTCP_WORK_RTX and MPTCP_RETRANSMIT are set only by
> mptcp_retransmit_timer(), so does that mean __mptcp_retrans() can never run
> for such a socket, leaving data reinjected into msk->rtx_queue when a
> subflow stalls or dies unretransmitted, and DATA_FIN retransmission
> disabled, while the write side keeps its sndbuf pinned?
> 
> The previous guard read the live socket state, so it stopped applying on the
> next state transition. Would clearing the bit in __mptcp_init_sock(), and on
> the connect()/listen() retry paths, restore that property?
Both remarks here are a combo of pre-existing races and behaviour change
with this patch. Still I think they should be addresses within the same
scope. A new revision will be needed.

/P