Re: [PATCH v4] mptcp: push queued data on passive TFO subflows becoming established

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: data race in mptcp_subflow_context bitfield on close_event_done
--

commit 5833b858d2930ce8da576be275d9bd667c3816cd
Author: T S Rameshkumar <rameshsv06@gmail.com>

mptcp: push queued data on passive TFO subflows becoming established

With TCP Fast Open on an MPTCP listener, if the server application
writes data while the passive subflow is still in SYN_RECV,
__mptcp_subflow_active() refuses transmission and the data is queued
into the msk write queue.

Closes: https://lore.kernel.org/netdev/CAFPPu1gU2Y-D+d4i3F0MoNkYK+e1U+=X3qf6QycjfKBw+8snPg@mail.gmail.com/

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 0384d6a023f9d..0e44de1298007 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -582,15 +582,15 @@ struct mptcp_subflow_context {
>  		disposable : 1,	    /* ctx can be free at ulp release time */
>  		closing : 1,	    /* must not pass rx data to msk anymore */
>  		valid_csum_seen : 1,        /* at least one csum validated */
> -		is_mptfo : 1,	    /* subflow is doing TFO */
>  		close_event_done : 1,       /* has done the post-closed part */

[Severity: High]
Does leaving close_event_done in this bitfield still result in a data race?

In net/mptcp/protocol.c:mptcp_close_ssk(), subflow->close_event_done is set
to true while holding only the msk lock.

Concurrently, the TCP RX path calls subflow_data_ready() ->
mptcp_subflow_data_available() -> subflow_check_data_avail() in
net/mptcp/subflow.c, which sets subflow->map_valid = 1 holding only the
ssk (subflow) lock.

Since close_event_done and map_valid are part of the same u32 bitfield,
could these uncoordinated read-modify-write operations cause concurrent
updates to be silently lost, potentially dropping valid data or executing
mptcp_close_ssk() multiple times?

>  		mpc_drop : 1,	    /* the MPC option has been dropped in a rtx */
>  		resetting : 1,	    /* subflow is resetting */
> -		__unused : 8;
> +		__unused : 9;
>  	bool	data_avail;
>  	bool	scheduled;
>  	bool	pm_listener;	    /* a listener managed by the kernel PM? */
>  	bool	fully_established;  /* path validated */
> +	bool	is_mptfo;	    /* subflow is doing TFO */
>  	u32	lent_mem_frag;
>  	u32	remote_nonce;
>  	u64	thmac;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008124220.213016-1-rameshkumar.t@phytecembedded.in?part=1