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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race in mptcp_pm_data_reset() when clearing the pm->reset struct group locklessly, causing torn reads/writes with concurrent PM operations.
--

commit 0c791790f3c719050b1d2004c347405ffb76512f
Author: Tao Cui <cuitao@kylinos.cn>

mptcp: pm: skip extra_subflows accounting on disconnected msk

This commit addresses a remotely triggerable panic caused by a WARN_ON_ONCE
guard on extra_subflows underflow. It skips the PM accounting when the socket
is in TCP_CLOSE, as the counters may have already been cleared by
mptcp_pm_data_reset().

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

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 9881092c16e78..520f0c638e6f7 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -671,9 +671,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
> +			 */

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race when
clearing the pm->reset struct group locklessly in mptcp_pm_data_reset()?

Looking at net/mptcp/protocol.c:mptcp_disconnect(), it calls
mptcp_pm_data_reset() on an active, network-exposed socket without acquiring
pm->lock:

net/mptcp/protocol.c:mptcp_disconnect() {
    ...
    mptcp_pm_data_reset(msk);
    ...
}

Inside net/mptcp/pm.c:mptcp_pm_data_reset(), the code does a lockless memset
of the struct group:

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

Could this cause torn reads and writes with concurrent PM operations?

For example, mptcp_pm_allow_new_subflow() concurrently modifies the memory
while holding pm->lock, but without holding the socket lock:

net/mptcp/pm.c:mptcp_pm_allow_new_subflow() {
    ...
    pm->extra_subflows++;
    ...
}

If a client socket initiates a local disconnect via connect(AF_UNSPEC), and
concurrently an in-flight SYN-ACK or remote MP_JOIN request is processed in
softirq context via mptcp_finish_join(), could they interleave?

It seems this might lead to data corruption of the pm->reset struct group
fields (including extra_subflows, accept_subflow, and addr_signal), which
could cause PM misbehavior such as permanent rejection of new subflows on a
reconnected socket.

[ ... ]

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