Re: [PATCH net v2 01/15] 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:

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.

[ ... ]

[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