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

[netdev-bot+sashiko@kernel.org]

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 <pabeni@redhat.com>

[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?

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index be59651e708e..d611af2eb74f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3588,6 +3588,7 @@ static void mptcp_destroy_common(struct mptcp_sock *msk)
>  
>  static int mptcp_disconnect(struct sock *sk, int flags)
>  {
> +	struct inet_connection_sock *icsk = inet_csk(sk);
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  
>  	/* We are on the fastopen error path. We can't call straight into the
> @@ -3600,8 +3601,13 @@ static int mptcp_disconnect(struct sock *sk, int flags)
>  	mptcp_check_listen_stop(sk);
>  	mptcp_set_state(sk, TCP_CLOSE);
>  
> -	mptcp_stop_rtx_timer(sk);
> -	mptcp_stop_tout_timer(sk);
> +	/* The later subflow close can not kick again the tout timer,
> +	 * as the msk is already in closed status.
> +	 */
> +	msk->timer_ival = icsk->icsk_rto_min;
> +	sk_stop_timer_sync(sk, &sk->mptcp_retransmit_timer);
> +	icsk->icsk_mtup.probe_timestamp = 0;
> +	sk_stop_timer_sync(sk, &icsk->mptcp_tout_timer);

[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