[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

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 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

[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 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 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