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

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

[T S Rameshkumar]

With TCP Fast Open on an MPTCP listener, if the server application
writes data while the passive subflow is still in SYN_RECV (after
consuming the client's SYN data but before the MP_CAPABLE third ACK
arrives), __mptcp_subflow_active() refuses transmission and the data
is queued into the msk write queue.

When the MPC third ACK arrives, the subflow transitions to
TCP_ESTABLISHED, but because the third ACK carries no DSS data,
the queued bytes remain stranded until the peer sends more data.

Fix this in subflow_state_change() by checking if the subflow was doing
passive TFO (subflow->is_mptfo) and has reached TCP_ESTABLISHED. Acquire
mptcp_data_lock() and call __mptcp_check_push() to flush queued bytes,
clearing is_mptfo so subsequent state transitions are ignored.

Reported-by: Petar Sakic <petar.sakic@ink.fish>
Closes: https://lore.kernel.org/netdev/CAFPPu1gU2Y-D+d4i3F0MoNkYK+e1U+=X3qf6QycjfKBw+8snPg@mail.gmail.com/
Fixes: fb7084501a61 ("mptcp: add support for TCP_FASTOPEN sockopt")
Signed-off-by: T S Rameshkumar <rameshkumar.t@phytecembedded.in>
---
 net/mptcp/options.c | 7 +++++++
 net/mptcp/subflow.c | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index ce0de02f5..d5238fa11 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);
 	mptcp_data_unlock((struct sock *)msk);
 
 check_notify:
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index f0a6725d2..c71122842 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;
+		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.
-- 
2.34.1

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

[sashiko-bot]

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

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

[MPTCP CI]

Hi T,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/37764365241

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/f72e0b60f8ac
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1181531


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)