[PATCH net 0/2] mptcp: misc fixes for v7.3-rc4

[PATCH net 0/2] mptcp: misc fixes for v7.3-rc4

[Matthieu Baerts (NGI0)]

Here are two unrelated fixes:

- Patch 1: avoid unneeded actions on subflow reset. A fix for another
  fix introduced in v6.12 and targeting a commit from v5.7.

- Patch 2: close a possible race when scheduling a closing path. A fix
  for another fix introduced in v6.0 and targeting v5.10.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Paolo Abeni (2):
      mptcp: avoid unneeded actions on subflow reset
      mptcp: close race between scheduler and state change

 net/mptcp/protocol.c | 4 +++-
 net/mptcp/protocol.h | 3 ++-
 net/mptcp/subflow.c  | 8 ++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)
---
base-commit: 83a945a529d6e002dd7339c532288a931f463dba
change-id: 20260915-net-mptcp-misc-fixes-7-3-rc4-538b9d7ec047

Best regards,
--  
Matthieu Baerts (NGI0) <matttbe@kernel.org>

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

[Matthieu Baerts (NGI0)]

From: Paolo Abeni <pabeni@redhat.com>

Once in a blue moon, the mptcp receive path can recursively call
mptcp_data_ready() via state change under unlucky error conditions, and
then try to hold the data lock again.

Break the recursion loop explicitly checking for the exceptional
condition.

Add a new flag instead of using an existing one like 'closing', to exit
early in subflow_state_change(). This avoids unneeded processing to
check for available data -- calling get_mapping_status() and more on a
dying subflow -- but also in error reporting and worker scheduling.

Fixes: e32d262c89e2 ("mptcp: handle consistently DSS corruption")
Cc: stable@vger.kernel.org
Reported-by: Xinyang Ge <xinyang@anthropic.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.h | 3 ++-
 net/mptcp/subflow.c  | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 2b4c27426477..0384d6a023f9 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 */
+		__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 01db7edce18a..cbe227d21844 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -438,6 +438,7 @@ void mptcp_subflow_reset(struct sock *ssk)
 	/* must hold: tcp_done() could drop last reference on parent */
 	sock_hold(sk);
 
+	subflow->resetting = 1;
 	mptcp_send_active_reset_reason(ssk);
 	tcp_done(ssk);
 	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags))
@@ -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;
+
 	/* 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.

-- 
2.55.0

[PATCH net 2/2] mptcp: close race between scheduler and state change

[Matthieu Baerts (NGI0)]

From: Paolo Abeni <pabeni@redhat.com>

The mptcp scheduler may race with subflow sockets state change: data
transmission on the selected socket may fail and a later release could
try to use mss_now reset to 0 for a divide operation.

Address the issue by explicitly checking for the critical scenario.

Fixes: c886d70286bf ("mptcp: do not queue data on closed subflows")
Cc: stable@vger.kernel.org
Reported-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
Reported-by: Xinyang Ge <xinyang@anthropic.com>
Closes: https://lore.kernel.org/20260525194828.1137119-1-shardul.b@mpiricsoftware.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0098e2830931..709610db3ce9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1664,7 +1664,9 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 
 static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info)
 {
-	tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
+	if (info->mss_now)
+		tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle,
+			 info->size_goal);
 	release_sock(ssk);
 }
 

-- 
2.55.0

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

[sashiko-bot]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 1 · Low: 0

- [High] The new unconditional `if (subflow->resetting) return;` in
  subflow_state_change() (net/mptcp/subflow.c:1891) removes the last…
- [High] mptcp: data race in bitfield resetting and close_event_done
- [Medium] The new early return in subflow_state_change()
  (net/mptcp/subflow.c:1891) is justified by an in-code comment claiming…

--

mptcp: avoid unneeded actions on subflow reset

This adds a new subflow->resetting bit, set in mptcp_subflow_reset() before
tcp_done(), and uses it to return early from subflow_state_change() so the
nested mptcp_data_ready() recursion is broken. The early return also skips
the data-available check, the error report and subflow_sched_work_if_closed().

> 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
> @@ -438,6 +438,7 @@ void mptcp_subflow_reset(struct sock *ssk)
>  	/* must hold: tcp_done() could drop last reference on parent */
>  	sock_hold(sk);
>  
> +	subflow->resetting = 1;
>  	mptcp_send_active_reset_reason(ssk);
>  	tcp_done(ssk);
>  	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags))

[Severity: High]
The new bit is carved out of the same 32-bit bitfield word as
closeing/close_event_done/mpc_drop, and a C bitfield store is a
read-modify-write of that whole word, not of the single bit.  So the
question is whether that word is only ever written under one lock.

mptcp_subflow_reset() now writes subflow->resetting.  On the
mptcp_mp_fail_no_response() path the caller is mptcp_worker(), which
holds the msk socket lock and then lock_sock_fast(ssk), so both locks
are held.  But the DSS-corruption path -- mptcp_dss_corruption() ->
mptcp_subflow_reset() -- runs from get_mapping_status() /
mptcp_subflow_data_available() in the ssk receive path, i.e. with only
the subflow socket lock (and possibly in BH context).  Meanwhile
mptcp_close_ssk() writes subflow->close_event_done = true holding only
the msk socket lock.  Those two locks do not exclude each other, so
the two load/or/store sequences can interleave on the same word.

If the msk-side store wins, the resetting = 1 store is silently
dropped, subflow_state_change() takes the old path and we are back to
the recursive mptcp_data_ready() and the data lock re-acquisition this
patch is meant to break -- the failure mode is the same deadlock, just
rarer and harder to reproduce.  If the reset-side store wins,
close_event_done is lost and the post-close event handling runs twice.
KCSAN should be able to flag this write/write pair.

Is there serialization here that I am missing for the
mptcp_dss_corruption() caller?  If not, would you consider keeping
resetting out of the msk-owned bitfield word -- a plain bool next to
data_avail/scheduled, or a bit in a flags word updated with set_bit()
-- so that setting it under the ssk lock cannot clobber bits owned by
the msk lock?

[ ... ]

> @@ -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 a reset subflow with a non-empty ssk->sk_receive_queue still get
cleaned up after this early return?

mptcp_subflow_reset() calls tcp_done(ssk), and tcp_set_state(TCP_CLOSE)
unhashes the socket, so subflow_data_ready() cannot fire again for this
ssk.  That makes the tcp_done() -> sk_state_change() ->
subflow_state_change() path the last remaining entry into
mptcp_subflow_data_available() for this subflow, and the new return skips
it.

The flush that exists for exactly this case lives in validate_data_csum():

net/mptcp/subflow.c:validate_data_csum() {
	...
	if (skb_queue_is_last(&ssk->sk_receive_queue, skb)) {
		/* if this subflow is closed, the partial mapping
		 * will be never completed; flush the pending skbs, so
		 * that subflow_sched_work_if_closed() can kick in
		 */
		if (unlikely(ssk->sk_state == TCP_CLOSE))
			while ((skb = skb_peek(&ssk->sk_receive_queue)))
				sk_eat_skb(ssk, skb);

		/* not enough data to validate the csum */
		return MAPPING_EMPTY;
	}
	...
}

With the skbs left in place, does __mptcp_close_subflow() keep skipping
the subflow forever?

net/mptcp/protocol.c:__mptcp_close_subflow() {
	...
		/* 'subflow_data_ready' will re-sched once rx queue is empty */
		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
			continue;

		mptcp_close_ssk(sk, ssk, subflow);
	...
}

mptcp_worker() has already cleared MPTCP_WORK_CLOSE_SUBFLOW at that point,
and recvmsg() only spools msk->backlog_list in mptcp_move_skbs() /
__mptcp_move_skbs(), so it never touches ssk->sk_receive_queue.

A concrete path with net.mptcp.checksum_enabled=1: a bad DSS csum arms
subflow->fail_tout in mptcp_subflow_fail(); a later partially received
mapping returns MAPPING_EMPTY leaving skbs queued on msk->first; the
MP_FAIL echo never arrives, so mptcp_worker() calls
mptcp_mp_fail_no_response():

net/mptcp/protocol.c:mptcp_mp_fail_no_response() {
	...
	slow = lock_sock_fast(ssk);
	mptcp_subflow_reset(ssk);
	WRITE_ONCE(mptcp_subflow_ctx(ssk)->fail_tout, 0);
	unlock_sock_fast(ssk, slow);
}

fail_tout is then cleared, and since __mptcp_close_ssk() is never reached,
mptcp_start_tout_timer() is never called either.  Does that leave the msk
in TCP_ESTABLISHED with its only, dead subflow, no pending timer, and a
reader blocked in mptcp_recvmsg() that is never woken with EOF or an
error?

The same early return also appears to leave reset join subflows (the
'reset:' label in check_fully_established() and __mptcp_flush_join_list())
with their context and queued skbs pinned on msk->conn_list.

For reference, the DSS-corruption path itself looks unaffected, because
__mptcp_move_skbs_from_subflow() re-calls mptcp_subflow_data_available(ssk)
in its own loop.

[Severity: Medium]
The comment states "subflow reset can't happen in case of fallback:
subflow_sched_work_if_closed() would be a no-op".  Is that premise
enforced anywhere?

mptcp_mp_fail_no_response() resets msk->first purely on fail_tout expiry,
with no __mptcp_check_fallback() test.  fail_tout is armed by
mptcp_subflow_fail() while the msk is not yet in fallback, and nothing
seems to clear it when a later fallback succeeds: __mptcp_try_fallback()
only touches allow_subflows / MPTCP_FALLBACK_DONE, and the pm.c side clears
fail_tout only on an MP_FAIL echo.

With net.mptcp.checksum_enabled=1: a middlebox mangles the MPTCP options,
MAPPING_BAD_CSUM with valid_csum_seen sends MP_FAIL and arms
fail_tout = jiffies + TCP_RTO_MAX; the echo never arrives; a subsequent
MAPPING_NODSS on the same first subflow makes mptcp_try_fallback() succeed,
so MPTCP_FALLBACK_DONE is set while fail_tout is still armed.  The tout
timer then runs mptcp_worker() -> mptcp_mp_fail_no_response() ->
mptcp_subflow_reset(msk->first) with __mptcp_check_fallback(msk) true.

In that state tcp_done() sets SHUTDOWN_MASK/TCP_CLOSE, so subflow_is_done(ssk)
is true and msk->first == ssk, which is precisely the branch that is not a
no-op:

net/mptcp/subflow.c:subflow_sched_work_if_closed() {
	...
	/* when the fallback subflow closes the rx side, trigger a 'dummy'
	 * ingress data fin, so that the msk state will follow along
	 */
	if (__mptcp_check_fallback(msk) && subflow_is_done(ssk) &&
	    msk->first == ssk &&
	    mptcp_update_rcv_data_fin(msk, subflow->map_seq +
				      subflow->map_data_len, true))
		mptcp_schedule_work(sk);
}

Skipping that call means mptcp_update_rcv_data_fin() is never invoked, the
msk never moves to CLOSE_WAIT/RCV_SHUTDOWN, and blocked readers get no EOF
until the close timeout (net.mptcp.close_timeout, 60 s by default) fires
and mptcp_do_fastclose() sets TCP_CLOSE and purges the backlog instead of
delivering a clean EOF.  Should the comment be qualified, or should the
check be narrowed so the fallback DATA_FIN injection still runs?

>  	/* 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://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-net-mptcp-misc-fixes-7-3-rc4-v1-0-a257797dd5d0%40kernel.org

Re: [PATCH net 2/2] mptcp: close race between scheduler and state change

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete fix: the identical `mss_now == 0` divide-by-zero that
  the patch guards in `mptcp_push_release()` (net/mptcp/protocol.c:1667)…

--

mptcp: close race between scheduler and state change

The patch guards the tcp_push() call in mptcp_push_release() with a check
for info->mss_now being non-zero, so that a subflow which failed
transmission (leaving mss_now at 0) is not used for a divide operation.

Closes: https://lore.kernel.org/20260525194828.1137119-1-shardul.b@mpiricsoftware.com

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0098e28309318..709610db3ce9d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1664,7 +1664,9 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>  
>  static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info)
>  {
> -	tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
> +	if (info->mss_now)
> +		tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle,
> +			 info->size_goal);
>  	release_sock(ssk);
>  }

[Severity: High]
Should the same guard also be added to the second tcp_push() call site, in
__mptcp_subflow_push_pending()?

That function starts with a zeroed info:

net/mptcp/protocol.c:__mptcp_subflow_push_pending() {
	struct mptcp_sendmsg_info info = {
		.data_lock_held = true,
	};
	...
	int copied = 0;

and info.mss_now is only ever assigned inside mptcp_sendmsg_frag(), after
the early return the commit message refers to:

	if (unlikely(!__tcp_can_send(ssk)))
		return -EAGAIN;

	/* compute send limit */
	...
	info->mss_now = tcp_send_mss(ssk, &info->size_goal, info->flags);

In the non-first iteration, the return value is accumulated into copied
without filtering negatives:

net/mptcp/protocol.c:__mptcp_subflow_push_pending() {
		if (READ_ONCE(subflow->scheduled)) {
			mptcp_subflow_set_scheduled(subflow, false);
			ret = __subflow_push_pending(sk, ssk, &info);
			if (ret <= 0)
				keep_pushing = false;
			copied += ret;
		}

__subflow_push_pending() passes mptcp_sendmsg_frag()'s -EAGAIN through
verbatim when the first frag fails, so copied becomes -EAGAIN, which is
non-zero, while info.mss_now is still 0:

out:
	if (copied) {
		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
			 info.size_goal);

Can that reach the divide the patch is trying to avoid?  With a non-empty
subflow write queue, tcp_push() does not take the autocork early return
(it requires skb->len < size_goal, i.e. skb->len < 0), so it calls
__tcp_push_pending_frames(sk, 0, nonagle), which only bails for TCP_CLOSE.
For FIN_WAIT1/FIN_WAIT2/CLOSING/LAST_ACK it continues into
tcp_write_xmit(sk, 0, ...) and tcp_tso_segs() -> tcp_tso_autosize():

net/ipv4/tcp_output.c:tcp_tso_autosize() {
	return max_t(u32, bytes / mss_now, min_tso_segs);

tcp_set_skb_tso_segs() has the same shape with
DIV_ROUND_UP(skb->len, mss_now).

Is the "scheduled but no longer sendable" state reachable on this path?
mptcp_sched_get_send() returns early without re-validating sendability if
any subflow already has the flag set:

net/mptcp/sched.c:mptcp_sched_get_send() {
	mptcp_for_each_subflow(msk, subflow) {
		if (READ_ONCE(subflow->scheduled))
			return 0;
	}

and __mptcp_subflow_push_pending() itself leaves the flag set on the
subflows it delegates via mptcp_subflow_delegate().  The tree also comments
on leftover flags elsewhere ("there could be left-over scheduled subflows;
clear them all or later xmit could use bad ones"), and
__mptcp_push_pending() already anticipates -EAGAIN together with
FIN_WAIT1/FIN_WAIT2 - which are exactly the states where
__tcp_push_pending_frames() does not bail out.

The entry point with first == false is __mptcp_check_push():

	if (!sock_owned_by_user(sk))
		__mptcp_subflow_push_pending(sk, ssk, false);

reached from ack_update_msk() in net/mptcp/options.c on incoming DSS acks,
so this runs in softirq context on peer-supplied input.

A second, smaller question on the same statement: since copied mixes byte
counts with negative errnos, can a positive count followed by an error that
sums to exactly zero (for example 11 bytes then -EAGAIN) make the
if (copied) test false and skip both the flush and
mptcp_reset_rtx_timer() even though data was queued?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-net-mptcp-misc-fixes-7-3-rc4-v1-0-a257797dd5d0%40kernel.org

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

[Paolo Abeni]

On 9/16/26 22:45, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 2 · Medium: 1 · Low: 0
> 
> - [High] The new unconditional `if (subflow->resetting) return;` in
>    subflow_state_change() (net/mptcp/subflow.c:1891) removes the last…
> - [High] mptcp: data race in bitfield resetting and close_event_done
> - [Medium] The new early return in subflow_state_change()
>    (net/mptcp/subflow.c:1891) is justified by an in-code comment claiming…
> 
> --
> 
> mptcp: avoid unneeded actions on subflow reset
> 
> This adds a new subflow->resetting bit, set in mptcp_subflow_reset() before
> tcp_done(), and uses it to return early from subflow_state_change() so the
> nested mptcp_data_ready() recursion is broken. The early return also skips
> the data-available check, the error report and subflow_sched_work_if_closed().
> 
>> 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
>> @@ -438,6 +438,7 @@ void mptcp_subflow_reset(struct sock *ssk)
>>   	/* must hold: tcp_done() could drop last reference on parent */
>>   	sock_hold(sk);
>>   
>> +	subflow->resetting = 1;
>>   	mptcp_send_active_reset_reason(ssk);
>>   	tcp_done(ssk);
>>   	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags))
> 
> [Severity: High]
> The new bit is carved out of the same 32-bit bitfield word as
> closeing/close_event_done/mpc_drop, and a C bitfield store is a
> read-modify-write of that whole word, not of the single bit.  So the
> question is whether that word is only ever written under one lock.
> 
> mptcp_subflow_reset() now writes subflow->resetting.  On the
> mptcp_mp_fail_no_response() path the caller is mptcp_worker(), which
> holds the msk socket lock and then lock_sock_fast(ssk), so both locks
> are held.  But the DSS-corruption path -- mptcp_dss_corruption() ->
> mptcp_subflow_reset() -- runs from get_mapping_status() /
> mptcp_subflow_data_available() in the ssk receive path, i.e. with only
> the subflow socket lock (and possibly in BH context).  Meanwhile
> mptcp_close_ssk() writes subflow->close_event_done = true holding only
> the msk socket lock.  Those two locks do not exclude each other, so
> the two load/or/store sequences can interleave on the same word.
> 
> If the msk-side store wins, the resetting = 1 store is silently
> dropped, subflow_state_change() takes the old path and we are back to
> the recursive mptcp_data_ready() and the data lock re-acquisition this
> patch is meant to break -- the failure mode is the same deadlock, just
> rarer and harder to reproduce.  If the reset-side store wins,
> close_event_done is lost and the post-close event handling runs twice.
> KCSAN should be able to flag this write/write pair.
> 
> Is there serialization here that I am missing for the
> mptcp_dss_corruption() caller?  If not, would you consider keeping
> resetting out of the msk-owned bitfield word -- a plain bool next to
> data_avail/scheduled, or a bit in a flags word updated with set_bit()
> -- so that setting it under the ssk lock cannot clobber bits owned by
> the msk lock?

This is actually a pre-exiting issue, and quite unrelated. Could deserve 
a separate/later fix.

> [ ... ]
> 
>> @@ -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 a reset subflow with a non-empty ssk->sk_receive_queue still get
> cleaned up after this early return?
> 
> mptcp_subflow_reset() calls tcp_done(ssk), and tcp_set_state(TCP_CLOSE)
> unhashes the socket, so subflow_data_ready() cannot fire again for this
> ssk.  That makes the tcp_done() -> sk_state_change() ->
> subflow_state_change() path the last remaining entry into
> mptcp_subflow_data_available() for this subflow, and the new return skips
> it.
> 
> The flush that exists for exactly this case lives in validate_data_csum():
> 
> net/mptcp/subflow.c:validate_data_csum() {
> 	...
> 	if (skb_queue_is_last(&ssk->sk_receive_queue, skb)) {
> 		/* if this subflow is closed, the partial mapping
> 		 * will be never completed; flush the pending skbs, so
> 		 * that subflow_sched_work_if_closed() can kick in
> 		 */
> 		if (unlikely(ssk->sk_state == TCP_CLOSE))
> 			while ((skb = skb_peek(&ssk->sk_receive_queue)))
> 				sk_eat_skb(ssk, skb);
> 
> 		/* not enough data to validate the csum */
> 		return MAPPING_EMPTY;
> 	}
> 	...
> }
> 
> With the skbs left in place, does __mptcp_close_subflow() keep skipping
> the subflow forever?
> 
> net/mptcp/protocol.c:__mptcp_close_subflow() {
> 	...
> 		/* 'subflow_data_ready' will re-sched once rx queue is empty */
> 		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
> 			continue;
> 
> 		mptcp_close_ssk(sk, ssk, subflow);
> 	...
> }
> 
> mptcp_worker() has already cleared MPTCP_WORK_CLOSE_SUBFLOW at that point,
> and recvmsg() only spools msk->backlog_list in mptcp_move_skbs() /
> __mptcp_move_skbs(), so it never touches ssk->sk_receive_queue.
> 
> A concrete path with net.mptcp.checksum_enabled=1: a bad DSS csum arms
> subflow->fail_tout in mptcp_subflow_fail(); a later partially received
> mapping returns MAPPING_EMPTY leaving skbs queued on msk->first; the
> MP_FAIL echo never arrives, so mptcp_worker() calls
> mptcp_mp_fail_no_response():
> 
> net/mptcp/protocol.c:mptcp_mp_fail_no_response() {
> 	...
> 	slow = lock_sock_fast(ssk);
> 	mptcp_subflow_reset(ssk);
> 	WRITE_ONCE(mptcp_subflow_ctx(ssk)->fail_tout, 0);
> 	unlock_sock_fast(ssk, slow);
> }
> 
> fail_tout is then cleared, and since __mptcp_close_ssk() is never reached,
> mptcp_start_tout_timer() is never called either.  Does that leave the msk
> in TCP_ESTABLISHED with its only, dead subflow, no pending timer, and a
> reader blocked in mptcp_recvmsg() that is never woken with EOF or an
> error?
> 
> The same early return also appears to leave reset join subflows (the
> 'reset:' label in check_fully_established() and __mptcp_flush_join_list())
> with their context and queued skbs pinned on msk->conn_list.
> 
> For reference, the DSS-corruption path itself looks unaffected, because
> __mptcp_move_skbs_from_subflow() re-calls mptcp_subflow_data_available(ssk)
> in its own loop.

This may deserve to be fixed in the same series.

> [Severity: Medium]
> The comment states "subflow reset can't happen in case of fallback:
> subflow_sched_work_if_closed() would be a no-op".  Is that premise
> enforced anywhere?
> 
> mptcp_mp_fail_no_response() resets msk->first purely on fail_tout expiry,
> with no __mptcp_check_fallback() test.  fail_tout is armed by
> mptcp_subflow_fail() while the msk is not yet in fallback, and nothing
> seems to clear it when a later fallback succeeds: __mptcp_try_fallback()
> only touches allow_subflows / MPTCP_FALLBACK_DONE, and the pm.c side clears
> fail_tout only on an MP_FAIL echo.
> 
> With net.mptcp.checksum_enabled=1: a middlebox mangles the MPTCP options,
> MAPPING_BAD_CSUM with valid_csum_seen sends MP_FAIL and arms
> fail_tout = jiffies + TCP_RTO_MAX; the echo never arrives; a subsequent
> MAPPING_NODSS on the same first subflow makes mptcp_try_fallback() succeed,
> so MPTCP_FALLBACK_DONE is set while fail_tout is still armed.  The tout
> timer then runs mptcp_worker() -> mptcp_mp_fail_no_response() ->
> mptcp_subflow_reset(msk->first) with __mptcp_check_fallback(msk) true.
> 
> In that state tcp_done() sets SHUTDOWN_MASK/TCP_CLOSE, so subflow_is_done(ssk)
> is true and msk->first == ssk, which is precisely the branch that is not a
> no-op:
> 
> net/mptcp/subflow.c:subflow_sched_work_if_closed() {
> 	...
> 	/* when the fallback subflow closes the rx side, trigger a 'dummy'
> 	 * ingress data fin, so that the msk state will follow along
> 	 */
> 	if (__mptcp_check_fallback(msk) && subflow_is_done(ssk) &&
> 	    msk->first == ssk &&
> 	    mptcp_update_rcv_data_fin(msk, subflow->map_seq +
> 				      subflow->map_data_len, true))
> 		mptcp_schedule_work(sk);
> }
> 
> Skipping that call means mptcp_update_rcv_data_fin() is never invoked, the
> msk never moves to CLOSE_WAIT/RCV_SHUTDOWN, and blocked readers get no EOF
> until the close timeout (net.mptcp.close_timeout, 60 s by default) fires
> and mptcp_do_fastclose() sets TCP_CLOSE and purges the backlog instead of
> delivering a clean EOF.  Should the comment be qualified, or should the
> check be narrowed so the fallback DATA_FIN injection still runs?
Yeah sashiko does not like my wording, plus another unrelated race 
between fallback and mp_fail timeout.

/P

Re: [PATCH net 2/2] mptcp: close race between scheduler and state change

[Paolo Abeni]

On 9/16/26 22:45, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> - [High] Incomplete fix: the identical `mss_now == 0` divide-by-zero that
>    the patch guards in `mptcp_push_release()` (net/mptcp/protocol.c:1667)…
> 
> --
> 
> mptcp: close race between scheduler and state change
> 
> The patch guards the tcp_push() call in mptcp_push_release() with a check
> for info->mss_now being non-zero, so that a subflow which failed
> transmission (leaving mss_now at 0) is not used for a divide operation.
> 
> Closes: https://lore.kernel.org/20260525194828.1137119-1-shardul.b@mpiricsoftware.com
> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 0098e28309318..709610db3ce9d 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -1664,7 +1664,9 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>>   
>>   static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info)
>>   {
>> -	tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
>> +	if (info->mss_now)
>> +		tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle,
>> +			 info->size_goal);
>>   	release_sock(ssk);
>>   }
> 
> [Severity: High]
> Should the same guard also be added to the second tcp_push() call site, in
> __mptcp_subflow_push_pending()?
> 
> That function starts with a zeroed info:
> 
> net/mptcp/protocol.c:__mptcp_subflow_push_pending() {
> 	struct mptcp_sendmsg_info info = {
> 		.data_lock_held = true,
> 	};
> 	...
> 	int copied = 0;
> 
> and info.mss_now is only ever assigned inside mptcp_sendmsg_frag(), after
> the early return the commit message refers to:
> 
> 	if (unlikely(!__tcp_can_send(ssk)))
> 		return -EAGAIN;
> 
> 	/* compute send limit */
> 	...
> 	info->mss_now = tcp_send_mss(ssk, &info->size_goal, info->flags);
> 
> In the non-first iteration, the return value is accumulated into copied
> without filtering negatives:
> 
> net/mptcp/protocol.c:__mptcp_subflow_push_pending() {
> 		if (READ_ONCE(subflow->scheduled)) {
> 			mptcp_subflow_set_scheduled(subflow, false);
> 			ret = __subflow_push_pending(sk, ssk, &info);
> 			if (ret <= 0)
> 				keep_pushing = false;
> 			copied += ret;
> 		}
> 
> __subflow_push_pending() passes mptcp_sendmsg_frag()'s -EAGAIN through
> verbatim when the first frag fails, so copied becomes -EAGAIN, which is
> non-zero, while info.mss_now is still 0:
This is a pre-existing issue but it could deserve to be fixed in the 
same series.

/P