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

[netdev-bot+sashiko@kernel.org]

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