Re: [PATCH net v2 06/15] mptcp: prevent race between disconnect() and rtx

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] The commit message of "mptcp: prevent race between disconnect()
  and rtx" describes the change as only "Prevent the race using the…
- [Low] The commit carries "Fixes: b29fcfb54cd7 (\"mptcp: full disconnect
  implementation\")" (v5.18) for a real race that can leave a reused msk…

--

mptcp: prevent race between disconnect() and rtx

This replaces the mptcp_stop_rtx_timer() and mptcp_stop_tout_timer() calls in
mptcp_disconnect() with open-coded sk_stop_timer_sync() calls, so a
concurrently running retransmit timer cannot resurrect state after the
disconnect path has reset it.  No Link: or Closes: tags are present in the
commit header.

A couple of questions below, one about the changelog and one about the tags.

    mptcp: prevent race between disconnect() and rtx
    
    Sashiko noted that the two event can race, leading to inconsistent
    status. Prevent the race using the synchronous timer stop operation.
    
    Fixes: b29fcfb54cd7 ("mptcp: full disconnect implementation")
    Signed-off-by: Paolo Abeni [off-list ref]

[Severity: Low]
The Fixes: tag points at b29fcfb54cd7, which went in during v5.18, and the
race described can leave a reused msk with MPTCP_RETRANSMIT / MPTCP_WORK_RTX
set after mptcp_disconnect() has already cleared msk->flags and msk->cb_flags.

Should this also carry Cc: stable@vger.kernel.org?  Most of the other fixes in
this series do, including "mptcp: do not reschedule the RTX timer for fallback
sockets", whose MPTCP_RTX_ENABLED bit this change relies on so the timer cannot
be re-armed once the socket is in TCP_CLOSE.

Also, the report is credited to "Sashiko" in prose only.  Would it be possible
to add the matching Closes: or Link: tag, the way the sibling commits in the
series do?

[Severity: Low]
The changelog says the race is prevented "using the synchronous timer stop
operation", which reads like a straight sk_stop_timer() to sk_stop_timer_sync()
swap, but the open-coded version in mptcp_disconnect() also changes two other
pieces of state.  Could the message mention them?

First, msk->timer_ival now ends up as icsk->icsk_rto_min, while the helper it
replaces sets it to zero:

net/mptcp/protocol.c:mptcp_stop_rtx_timer() {
	sk_stop_timer(sk, &sk->mptcp_retransmit_timer);
	mptcp_sk(sk)->timer_ival = 0;
}

The new value is what __mptcp_init_sock() seeds, so it looks intentional and
harmless, and the only reader, mptcp_reset_rtx_timer(), is gated on
MPTCP_RTX_ENABLED which mptcp_set_state(TCP_CLOSE) has just cleared.  Is that
the reasoning?

Second, the tout timer is now stopped and probe_timestamp cleared
unconditionally, whereas the replaced helper bailed out early when
probe_timestamp was already zero:

net/mptcp/protocol.h:mptcp_stop_tout_timer() {
	if (!inet_csk(sk)->icsk_mtup.probe_timestamp)
		return;

	sk_stop_timer(sk, &inet_csk(sk)->mptcp_tout_timer);
	inet_csk(sk)->icsk_mtup.probe_timestamp = 0;
}

That early return means a tout timer armed only for an MP_FAIL timeout, where
mptcp_reset_tout_timer() left probe_timestamp at zero, stayed pending across
disconnect().  Is that a separate fix folded in here, and would it be worth
calling out in the changelog so stable backporters can see it?

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