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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Incomplete gate: mptcp_connect() (net/mptcp/protocol.c:4149)
  unconditionally does set_bit(MPTCP_RTX_ENABLED, &msk->flags) before…

--

mptcp: do not reschedule the RTX timer for fallback sockets

This adds a new MPTCP_RTX_ENABLED flag bit so that mptcp_reset_rtx_timer()
only arms the MPTCP retransmission timer while the msk has not fallen back
to plain TCP and is not closed.  The bit is set in mptcp_connect() and
mptcp_listen(), and cleared on fallback and on the transition to TCP_CLOSE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e1f08f71cdb1..be59651e708e 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -95,6 +95,7 @@ bool __mptcp_try_fallback(struct mptcp_sock *msk, int fb_mib)
>  
>  	msk->allow_subflows = false;
>  	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
> +	clear_bit(MPTCP_RTX_ENABLED, &msk->flags);
>  	__MPTCP_INC_STATS(net, fb_mib);
>  	spin_unlock_bh(&msk->fallback_lock);
>  	return true;
> @@ -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_ENABLED, &msk->flags))
>  		return;
>  
> -	tout = mptcp_sk(sk)->timer_ival;
> +	tout = msk->timer_ival;
>  	sk_reset_timer(sk, &sk->mptcp_retransmit_timer, jiffies + tout);
>  }
>  

[ ... ]

> @@ -4141,6 +4146,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
>  	if (IS_ERR(ssk))
>  		return PTR_ERR(ssk);
>  
> +	set_bit(MPTCP_RTX_ENABLED, &msk->flags);
>  	mptcp_set_state(sk, TCP_SYN_SENT);
>  	subflow = mptcp_subflow_ctx(ssk);

[Severity: Medium]
Can this set_bit() re-enable the RTX timer on a msk that is already in
fallback?  It runs unconditionally, before any of the fallback checks below
it, and the fallback state is sticky across a failed connect().

Sequence that seems reachable from userspace on its own fd:

1) First connect(): mptcp_active_should_disable() is true (or
   mptcp_token_new_connect() fails), so mptcp_connect() calls
   mptcp_early_fallback() -> __mptcp_try_fallback(), which sets
   MPTCP_FALLBACK_DONE, clears MPTCP_RTX_ENABLED and leaves
   subflow->request_mptcp == 0.

2) ssk->sk_prot->connect() fails immediately (for instance -EAFNOSUPPORT on
   a bad sin_family, or -ENETUNREACH from the route lookup), so
   mptcp_connect() takes:

	if (unlikely(err)) {
		/* avoid leaving a dangling token in an unconnected socket */
		mptcp_token_destroy(msk);
		mptcp_set_state(sk, TCP_CLOSE);
		return err;
	}

   MPTCP_RTX_ENABLED is cleared again by the TCP_CLOSE transition, but
   MPTCP_FALLBACK_DONE stays set.

3) __inet_stream_connect() returns the error directly:

	err = sk->sk_prot->connect(sk, uaddr, addr_len);
	if (err < 0)
		goto out;

   so sk->sk_prot->disconnect() (mptcp_disconnect(), which does
   WRITE_ONCE(msk->flags, 0)) is never called, and the socket stays
   SS_UNCONNECTED with sk_state == TCP_CLOSE.

4) Userspace calls connect() again.  mptcp_connect() sets
   MPTCP_RTX_ENABLED unconditionally, and since request_mptcp is already 0
   the fallback branches are skipped.  Even if mptcp_early_fallback() were
   reached, __mptcp_try_fallback() returns early at:

	if (__mptcp_check_fallback(msk))
		return true;

   before the new clear_bit(MPTCP_RTX_ENABLED, ...).

The result is a live fallback msk with MPTCP_RTX_ENABLED set, so
mptcp_reset_rtx_timer() passes the new gate and the send paths
(__mptcp_push_pending() and friends) arm the MPTCP RTX timer just as they
did before the patch:

		if (!mptcp_rtx_timer_pending(sk))
			mptcp_reset_rtx_timer(sk);

Would gating the set_bit() on !__mptcp_check_fallback(msk), or moving it
below the early-fallback checks, close this case and also make the new
comment "Prevent rescheduling on close and in case of fallback" hold for a
msk whose fallback predates the current connect()?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-net-mptcp-misc-fixes-7-3-rc1-v2-0-df1de70348b6%40kernel.org