Re: [PATCH] mptcp: serialize subflow->closing with RX path

[Matthieu Baerts <matttbe@kernel.org>]

Hi Kalpan,

On 07/05/2026 09:28, Kalpan Jani wrote:
> There is a race between mptcp_data_ready() (RX path) and
> mptcp_close_ssk() (teardown path) when accessing subflow->closing.

Thank you for sharing this patch!

Sadly, this patch doesn't apply and looks corrupted:

  Applying: mptcp: serialize subflow->closing with RX path
  error: corrupt patch at line 44
  error: could not build fake ancestor

Did you manually edit it without changing the line references?

While at it, please follow the rules from:

  https://docs.kernel.org/process/maintainer-netdev.html

=> designate your patch to a tree: [PATCH net]

It might be easier if you send new versions only to the MPTCP ML (not
ccing netdev).

> Currently, mptcp_data_ready() checks subflow->closing before acquiring
> mptcp_data_lock(), while mptcp_close_ssk() may concurrently set
> subflow->closing and purge backlog entries. This creates a classic
> time-of-check vs time-of-use (TOCTOU) race:
> 
>   CPU A (close path)              CPU B (RX path)
>   ----------------------         -------------------------
>   set closing = 1
>                                  read closing == 0
>   purge backlog
>                                  enqueue skb to backlog
> 
> As a result, skb entries referencing the subflow socket (ssk) may be
> enqueued after the subflow is marked closing and scheduled for cleanup.
> This can lead to:
> 
>   - WARN in inet_sock_destruct() due to non-zero sk_rmem_alloc
>   - potential use-after-free via stale skb->sk references

By chance, do you have (decoded) calltraces to share in the commit
message? And even better: a reproducer? Or explaining how you found this
issue, and eventually which tool helped you find it.

> Fix this by serializing both the closing check and backlog enqueue
> under mptcp_data_lock(). This ensures that subflow->closing state and
> backlog operations are observed atomically, preventing new skb from
> being enqueued once teardown begins.
> 
> Also protect backlog cleanup in mptcp_close_ssk() with the same lock
> to guarantee mutual exclusion with the RX path.
> 
> This restores proper synchronization between RX and teardown paths
> and prevents stale skb references to closing subflows.

Also, for fixes, the "Fixes:" tag is required.

> Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
> ---
>  net/mptcp/protocol.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 718e910ff..295f8e1c0 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -910,14 +910,34 @@ void mptcp_data_ready(struct sock *sk, struct sock *ssk)
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  
> +	/*
> +	 * The close path can set subflow->closing while we are racing
> +	 * from BH context here. The old check was done before taking
> +	 * mptcp_data_lock(), leaving a TOCTOU window:
> +	 *
> +	 *   CPU A: close path sets closing = 1 and purges backlog
> +	 *   CPU B: already observed closing == 0 and later enqueues skb
> +	 *
> +	 * That skb keeps skb->sk == ssk and can later trigger:
> +	 * - WARN in inet_sock_destruct() (ssk->sk_rmem_alloc != 0)
> +	 * - UAF in backlog purge via stale skb->sk
> +	 */

I don't think that's useful to add a comment referring an old behaviour.

> +
>  	/* The peer can send data while we are shutting down this
>  	 * subflow at subflow destruction time, but we must avoid enqueuing
>  	 * more data to the msk receive queue
>  	 */

Instead, I suggest moving this comment below as well, and merge it with
the new one you added.
> -	if (unlikely(subflow->closing))
> -		return;
>  
>  	mptcp_data_lock(sk);
> +
> +	/* Serialize closing check with backlog enqueue */
> +	if (unlikely(subflow->closing)) {
> +		mptcp_data_unlock(sk);

When locks are used, we usually prefer having one exit path: please add
a new label above mptcp_data_unlock() below, and a goto here.

> +		return;
> +	}
> +
>  	mptcp_rcv_rtt_update(msk, subflow);
>  	if (!sock_owned_by_user(sk)) {
>  		/* Wake-up the reader only for in-sequence data */
> @@ -2653,9 +2673,12 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	if (sk->sk_state == TCP_ESTABLISHED)
>  		mptcp_event(MPTCP_EVENT_SUB_CLOSED, mptcp_sk(sk), ssk, GFP_KERNEL);
>  
> -	/* Remove any reference from the backlog to this ssk; backlog skbs consume
> +	/* Remove any reference from the backlog to this ssk.
> +	 * Serialize cleanup with RX-side enqueue using mptcp_data_lock().

Easier to add this new line at the end of the comment to reduce the diff.

> +	 * Backlog skbs consume
>  	 * space in the msk receive queue, no need to touch sk->sk_rmem_alloc
>  	 */
> +	mptcp_data_lock(sk);
>  	list_for_each_entry(skb, &msk->backlog_list, list) {
>  		if (skb->sk != ssk)
>  			continue;
> @@ -2663,6 +2686,8 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		atomic_sub(skb->truesize, &skb->sk->sk_rmem_alloc);
>  		skb->sk = NULL;
>  	}
> +	mptcp_data_unlock(sk);
> +
>  

No double empty lines. I think 'checkpatch' would tell you that.

>  	/* subflow aborted before reaching the fully_established status
>  	 * attempt the creation of the next subflow

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.