Re: [PATCH net 1/2] mptcp: avoid unneeded actions on subflow reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: data race in bitfield resetting and close_event_done
- [High] mptcp: memory leak of receive queue skbs on subflow reset
--

commit c5cb7c23812480bd88951ebb970702ef4bea70fa
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: avoid unneeded actions on subflow reset

This commit introduces a resetting flag to break a recursion loop during
subflow reset. It avoids unnecessary Rx queue processing and error
reporting by exiting early in subflow_state_change() when the exceptional
condition is met.

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 2b4c274264773..0384d6a023f9d 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -585,7 +585,8 @@ struct mptcp_subflow_context {
>  		is_mptfo : 1,	    /* subflow is doing TFO */
>  		close_event_done : 1,       /* has done the post-closed part */
>  		mpc_drop : 1,	    /* the MPC option has been dropped in a rtx */
> -		__unused : 9;
> +		resetting : 1,	    /* subflow is resetting */

[Severity: High]
Does this bitfield placement introduce a data race?

The resetting and close_event_done flags share the same 32-bit word in struct
mptcp_subflow_context. In C, bitfield assignments compile to non-atomic
read-modify-write operations on the entire word.

It appears mptcp_subflow_reset() modifies resetting under the ssk lock, while
mptcp_close_ssk() modifies close_event_done under the parent sk lock. Since
these locks are distinct and not mutually exclusive, concurrent updates could
overwrite and lose the resetting flag, which might reintroduce the recursive
lock deadlock this patch aims to fix.

> +		__unused : 8;
>  	bool	data_avail;
>  	bool	scheduled;
>  	bool	pm_listener;	    /* a listener managed by the kernel PM? */

[ ... ]

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 01db7edce18a6..cbe227d218449 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c

[ ... ]

> @@ -1883,6 +1884,13 @@ static void subflow_state_change(struct sock *sk)
>  
>  	__subflow_state_change(sk);
>  
> +	/* Rx queue processing is unneeded, error reporting will take place at
> +	 * __mptcp_close_ssk() time and subflow reset can't happen in case of
> +	 * fallback: subflow_sched_work_if_closed() would be a no-op.
> +	 */
> +	if (subflow->resetting)
> +		return;
> +

[Severity: High]
Can this early return cause a memory leak of unread skbs?

When a subflow is reset, tcp_done() is called and sets the state to TCP_CLOSE,
but it does not purge the receive queue. Because this early return bypasses
mptcp_data_ready(), the skbs remaining in the receive queue are never drained.

Later, __mptcp_close_subflow() explicitly skips closing any subflow with a
non-empty receive queue:

if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
        continue;

Could this cause the subflow to be permanently skipped and kept in the
connection list, leaking both the subflow context and unread skbs until the
parent socket is closed?

>  	/* as recvmsg() does not acquire the subflow socket for ssk selection
>  	 * a fin packet carrying a DSS can be unnoticed if we don't trigger
>  	 * the data available machinery here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-net-mptcp-misc-fixes-7-3-rc4-v1-0-a257797dd5d0@kernel.org?part=1