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] net/mptcp: data race in subflow bitfields causes list corruption
--

commit 91c44c97db7bbddaa206314ca428111bec1fb73f
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, passive subflows that receive
data during SYN_RECV queue it into the msk write queue. This patch flushes
the queued bytes when the MPC third ACK arrives by checking if the subflow
was doing passive TFO.

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

> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1042,6 +1042,15 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>  
>  	mptcp_data_lock((struct sock *)msk);
>  	__mptcp_subflow_fully_established(msk, subflow, mp_opt);
> +	/* Passive TFO: the application may have written data while the
> +	 * subflow was still in SYN_RECV; __mptcp_subflow_active() refused
> +	 * it then and nothing else spools the msk write queue when the
> +	 * MPC third ack (no DSS) arrives. Push it now.
> +	 */
> +	if (subflow->is_mptfo) {
> +		subflow->is_mptfo = 0;

[Severity: High]
Could this bitfield modification race with mptcp_close_ssk() and corrupt
the conn_list linked list?

In check_fully_established(), the new modification to subflow->is_mptfo is
protected by mptcp_data_lock(msk) and lock_sock(ssk).

However, there are asynchronous paths (like mptcp_pm_nl_rm_subflow_received()
or mptcp_worker()) that call mptcp_close_ssk(). That function modifies the
subflow->close_event_done bitfield while holding only lock_sock(msk).

In struct mptcp_subflow_context, these two bitfields share the same memory
word:

        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 */
        mpc_drop : 1,               /* the MPC option has been dropped */

Since the lock_sock(msk) mutex does not overlap with the locks held in this
code block, concurrent execution could result in a read-modify-write data
race. 

If the close_event_done = true update is overwritten by is_mptfo = 0,
mptcp_close_ssk() could be re-executed on the same subflow later. This
would result in a double list_del(&subflow->node) inside __mptcp_close_ssk(),
corrupting the conn_list.

Is there a safe way to update is_mptfo without introducing a read-modify-write
race on the shared bitfield word?

> +		__mptcp_check_push((struct sock *)msk, ssk);
> +	}
>  	mptcp_data_unlock((struct sock *)msk);
>  
>  check_notify:

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