Re: [PATCH 5.15.y 1/2] mptcp: avoid unneeded actions on subflow reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: data race in subflow bitfield leads to deadlock
--

commit 748ca2aef871b6b237079df0a200a9001209199e
Author: Paolo Abeni <pabeni@redhat.com>
Subject: mptcp: avoid unneeded actions on subflow reset

This commit introduces a new flag, resetting, to exit early in
subflow_state_change(), preventing recursive calls to mptcp_data_ready()
and recursive spinlock acquisition. It also explicitly flushes the RX queue
at reset time.

Link: https://patch.msgid.link/20260917-net-mptcp-misc-fixes-7-3-rc4-v2-1-0cf5c72667c8@kernel.org

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 609e7768fab31..913d9b1474b05 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -447,7 +447,8 @@ struct mptcp_subflow_context {
>  		stale : 1,	    /* unable to snd/rcv data, do not use for xmit */
>  		valid_csum_seen : 1,        /* at least one csum validated */
>  		close_event_done : 1,       /* has done the post-closed part */
> -		__unused : 11;
> +		resetting : 1,	    /* subflow is resetting */
> +		__unused : 10;
>  	enum mptcp_data_avail data_avail;
>  	bool	pm_listener;	    /* a listener managed by the kernel PM? */
>  	u32	remote_nonce;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 33b16ea7ae5bc..5ee090b74fa45 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -373,6 +373,10 @@ void mptcp_subflow_reset(struct sock *ssk)
>  	/* must hold: tcp_done() could drop last reference on parent */
>  	sock_hold(sk);
>  
> +	subflow->resetting = 1;

[Severity: High]
Does this bitfield assignment introduce a data race that could clear the new
flag and cause a deadlock?

The C compiler implements bitfield modifications as non-atomic read-modify-write
operations on the containing 32-bit word. 

Here in mptcp_subflow_reset(), subflow->resetting = 1 is set while holding the
subflow ssk lock and the mptcp_data_lock, but the msk socket lock is not held.

Concurrently, a PM event like mptcp_pm_nl_mp_prio_send_ack() can modify
send_mp_prio in the same bitfield word:

net/mptcp/pm_netlink.c:mptcp_pm_nl_mp_prio_send_ack() {
    ...
    subflow->send_mp_prio = 1;
    subflow->request_bkup = bkup;
    ...
}

This PM path holds the msk socket lock and pm.lock, but critically does not
hold the ssk lock.

If the concurrent unguarded writes race, could the PM thread's update silently
overwrite and clear the resetting flag?

If the resetting flag is lost, the fix introduced in this commit would be
bypassed. subflow_state_change() would fail to return early and instead
recursively call mptcp_data_ready(), which tries to re-acquire the already held
mptcp_data_lock spinlock, leading to an immediate deadlock.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919202918.2074632-4-matttbe@kernel.org?part=1