Re: [PATCH mptcp-next 2/2] mptcp: pm: skip extra_subflows accounting on disconnected msk

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Subflows in `join_list` can be leaked due to unsynchronized zeroing of `cb_flags` in `mptcp_disconnect`.
- [High] Data race on `pm->reset` and `pm->rm_list_*` fields due to missing `pm->lock` in `mptcp_pm_data_reset()`.
--

commit 179deded54d27540d6b26de1228c24065e1fa9d4
Author: Tao Cui <cuitao@kylinos.cn>

mptcp: pm: skip extra_subflows accounting on disconnected msk

This commit addresses a WARN_ON_ONCE crash related to extra_subflows
underflow by skipping the PM accounting when the mptcp socket is in the
TCP_CLOSE state. It replaces the warnings with rate-limited prints and
avoids modifying the counters for disconnected sockets.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/629

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index b10895606542a..60276eb1ef384 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -676,9 +676,18 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
>  	update_subflows = subflow->request_join || subflow->mp_join;
>  	if (mptcp_pm_is_userspace(msk)) {
>  		if (update_subflows) {
> +			/* The PM counters have already been cleared if the
> +			 * msk got disconnected while this subflow was still
> +			 * queued in the join list
> +			 */
> +			if (inet_sk_state_load(sk) == TCP_CLOSE)
> +				return;

[Severity: High]
This is a pre-existing issue, but does mptcp_disconnect() leak subflows when
zeroing cb_flags non-atomically?

If a concurrent mptcp_finish_join() receives an MP_JOIN ACK for a subflow
while the parent socket is being disconnected, it can hit the
sock_owned_by_user(parent) branch. The subflow is added to msk->join_list
and MPTCP_FLUSH_JOIN_LIST is set in msk->cb_flags while holding the parent's
data lock.

Simultaneously, mptcp_disconnect() executes msk->cb_flags = 0 without holding
the mptcp_data_lock, erasing the MPTCP_FLUSH_JOIN_LIST flag:

net/mptcp/protocol.c:mptcp_disconnect() {
	...
	msk->cb_flags = 0;
	...
}

When the lock is released, release_sock() invokes mptcp_release_cb(), but
since the flag was erased, it skips the flush:

net/mptcp/protocol.c:mptcp_release_cb() {
	...
	if (flags & BIT(MPTCP_FLUSH_JOIN_LIST))
		__mptcp_flush_join_list(sk, &join_list);
	...
}

Can this lead to the sockets in join_list being completely leaked when
mptcp_destroy_common() later iterates only over conn_list?

[Severity: High]
This is a pre-existing issue, but is there a data race on pm->reset and
pm->rm_list_* fields when mptcp_pm_data_reset() is called during disconnect?

When mptcp_disconnect() sets sk->sk_state to TCP_CLOSE, it calls
mptcp_pm_data_reset(msk), which modifies PM structures without acquiring
pm->lock:

net/mptcp/pm.c:mptcp_pm_data_reset() {
	...
	memset(&pm->reset, 0, sizeof(pm->reset));
	pm->rm_list_tx.nr = 0;
	pm->rm_list_rx.nr = 0;
	...
}

At the same time, subflow receive paths can process incoming MP_JOIN or
RM_ADDR packets in BH context, holding only pm->lock and lock_sock(ssk) rather
than lock_sock(msk). For example:

net/mptcp/pm.c:mptcp_pm_allow_new_subflow() {
	...
	spin_lock_bh(&pm->lock);
	if (READ_ONCE(pm->accept_subflow)) {
		ret = pm->extra_subflows < limit_extra_subflows;
	...
}

Could this unprotected memset in mptcp_pm_data_reset() cause torn writes and
corrupted PM state when racing with readers and writers that only hold
pm->lock?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831093206.689827-1-cui.tao@linux.dev?part=2