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

[PATCH v3] 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 and is marked fully established in
check_fully_established(), but because the third ACK carries no
DSS data, the queued bytes remain stranded until the peer sends
more data.

Fix this in check_fully_established() by checking if the subflow
was doing passive TFO (subflow->is_mptfo). Under mptcp_data_lock(),
flush queued bytes via __mptcp_check_push() and clear is_mptfo.

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>
---
v2 -> v3:
 - Move the push and is_mptfo reset directly into check_fully_established()
   under mptcp_data_lock(), avoiding any potential race in subflow_state_change()
   which could clear is_mptfo before check_fully_established() processes the 3rd ACK.
v1 -> v2:
 - Use __mptcp_check_push() under mptcp_data_lock() instead of mptcp_push_pending().

 net/mptcp/options.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index ce0de02f5..b865ee6af 100644
--- 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;
+		__mptcp_check_push((struct sock *)msk, ssk);
+	}
 	mptcp_data_unlock((struct sock *)msk);
 
 check_notify:
-- 
2.34.1

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix only covers passive TFO children whose SYN carried data.
- [Medium] If the passive TFO subflow's third ACK has no MP_CAPABLE
  option (for example a middlebox strips MPTCP options from non-SYN…
- [Low] The author and sign-off addresses do not match.

--

mptcp: push queued data on passive TFO subflows becoming established

This patch handles data that the server application writes while a
passive TFO subflow is still in SYN_RECV. When the MPC third ACK makes
the subflow fully established in check_fully_established(), the patch
calls __mptcp_check_push() under mptcp_data_lock() if subflow->is_mptfo
is set, and then clears is_mptfo.

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

> Signed-off-by: T S Rameshkumar <rameshkumar.t@phytecembedded.in>

[Severity: Low]
This isn't a bug, but the author address (rameshsv06@gmail.com) does not
match the only Signed-off-by address (rameshkumar.t@phytecembedded.in).
checkpatch.pl reports this as FROM_SIGN_OFF_MISMATCH.

Could the sign-off or the From: line be changed so they match?

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

[Severity: High]
Does this cover passive TFO children created from a SYN that carried no
data?

Only mptcp_fastopen_subflow_synack_set_params() sets is_mptfo, and only
when SYN data is on the child's receive queue:

net/mptcp/fastopen.c:mptcp_fastopen_subflow_synack_set_params() {
    ...
	/* A valid TFO cookie does not guarantee SYN data. */
	skb = skb_peek(&ssk->sk_receive_queue);
	if (!skb)
		return;

	subflow->is_mptfo = 1;
    ...
}

tcp_try_fastopen() still creates a full TFO child in TCP_SYN_RECV from a
data-less SYN in two cases:

  - a SYN with a valid cookie and no payload
  - a cookie-request SYN when TFO_SERVER_COOKIE_NOT_REQD is set

The comment in tcp_try_fastopen() says:

	 * Note: Data-less SYN with valid cookie is allowed to
	 * send data in SYN_RECV state.

In both cases tcp_fastopen_add_skb() returns early on
end_seq == rcv_nxt. Nothing gets queued, so is_mptfo stays 0.

The msk is cloned in TCP_ESTABLISHED, so the server can accept() and
write right away. __mptcp_subflow_active() rejects the SYN_RECV ssk, so
the data stays at msk->first_pending.

When the pure MPC third ACK arrives, check_fully_established() reaches
set_fully_established but skips this push. mptcp_incoming_options() then
returns before ack_update_msk():

		if (!(mp_opt.suboptions & OPTION_MPTCP_DSS))
			return true;

Neither subflow_state_change() nor mptcp_pm_fully_established() pushes
the msk write queue. No rtx timer is armed either, because nothing was
ever sent.

Wouldn't a server-speaks-first protocol still stall in this case? That
is the same symptom the commit message describes.

Would it work better to trigger the push when a passive MPC subflow
becomes fully established while msk->first_pending is set, rather than
on is_mptfo?

[Severity: Medium]
What happens if the third ACK on a passive TFO subflow with SYN data has
no MP_CAPABLE option? That can happen if a middlebox strips MPTCP
options from non-SYN segments, or if the peer leaves them out.

The SYN payload advanced ssn_offset, so the third ACK has
seq == ssn_offset + 1 and skips the out-of-order branch. Without DSS or
ADD_ADDR, it then takes the fallback branch earlier in
check_fully_established():

	if (!(mp_opt->suboptions & OPTIONS_MPTCP_MPC)) {
		if (subflow->mp_join)
			goto reset;
		subflow->mp_capable = 0;
		if (!mptcp_try_fallback(ssk, MPTCP_MIB_MPCAPABLEDATAFALLBACK)) {
			...
		}
		return false;
	}

That branch returns before set_fully_established, so the new push never
runs. mptcp_incoming_options() then returns without pushing:

	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
		return sk->sk_state != TCP_CLOSE;

tcp_data_snd_check() finds the ssk write queue empty, because the data
is still in the msk write queue. __mptcp_check_push() only runs on the
next incoming segment, through the __mptcp_check_fallback() branch at
the top of mptcp_incoming_options().

If the client is waiting for the server's reply to its TFO request,
could the data written in SYN_RECV stay stuck in this fallback case too?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008104840.131986-1-rameshkumar.t%40phytecembedded.in