Re: [PATCH v2] 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: premature clear of is_mptfo breaks passive TFO sequence validation
--

commit 22ac3fb6b29817664eb5b68ac5a70275e689140e
Author: T S Rameshkumar <rameshsv06@gmail.com>

mptcp: push queued data on passive TFO subflows becoming established

This commit flushes queued data on passive TCP Fast Open subflows
when the subflow transitions to TCP_ESTABLISHED. It checks for
subflow->is_mptfo in subflow_state_change() and check_fully_established()
to push the data, and clears the flag so subsequent state transitions
are ignored.

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

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index ce0de02f5a3a1..d5238fa111dee 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1042,6 +1042,13 @@ 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)
> +		__mptcp_check_push((struct sock *)msk, ssk);

[Severity: High]
Does this block become entirely unreachable dead code?

When an MPTCP connection using passive TCP Fast Open receives the 3rd ACK,
tcp_rcv_state_process() executes sk_state_change() (transitioning from SYN_RECV
to ESTABLISHED) before executing tcp_data_queue().

Since the newly added code in subflow_state_change() clears the
is_mptfo flag during this state transition, subflow->is_mptfo will evaluate
to 0 by the time check_fully_established() is reached here.

>  	mptcp_data_unlock((struct sock *)msk);
>  
>  check_notify:
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index f0a6725d2c376..c7112284254a5 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1894,6 +1894,13 @@ static void subflow_state_change(struct sock *sk)
>  	if (subflow->resetting)
>  		return;
>  
> +	if (subflow->is_mptfo && sk->sk_state == TCP_ESTABLISHED) {
> +		subflow->is_mptfo = 0;

[Severity: High]
Does prematurely clearing the is_mptfo flag here break passive TFO sequence
validation?

After this state change runs, tcp_data_queue() calls mptcp_incoming_options()
which evaluates check_fully_established(). Because the flag is already 0, the
preexisting sequence number check bypass for passive TFO fails:

net/mptcp/options.c:check_fully_established() {
    ...
	if (TCP_SKB_CB(skb)->seq != subflow->ssn_offset + 1) {
		if (subflow->mp_join)
			goto reset;
		if (subflow->is_mptfo && mp_opt->suboptions & OPTION_MPTCP_MPC_ACK)
			goto set_fully_established;
		return subflow->mp_capable;
	}
    ...
}

The TFO 3rd ACK sequence number includes the data length sent in the SYN and
does not equal ssn_offset + 1. Since the bypass condition fails, doesn't this
prevent the subflow from becoming fully established?

> +		mptcp_data_lock(parent);
> +		__mptcp_check_push(parent, sk);
> +		mptcp_data_unlock(parent);
> +	}
> +
>  	/* 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/20261008100941.104715-1-rameshkumar.t@phytecembedded.in?part=1