Re: [PATCH mptcp-net 25/34] mptcp: push queued data on passive TFO subflows falling back to TCP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mptcp: incomplete fallback handling in
  check_fully_established()
--

commit 66148bc476eb2e96002647feaf23c977ed60ceb1
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: push queued data on passive TFO subflows falling back to TCP

This commit pushes pending data in check_fully_established() when a
passive TFO subflow falls back to plain TCP on the third ACK.

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 3477a7ec5a051..c56ca5e4df14b 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1035,6 +1035,11 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>  			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_FALLBACKFAILED);
>  			goto reset;
>  		}
> +		if (tcp_sk(ssk)->syn_fastopen_child) {
> +			mptcp_data_lock(sk);
> +			__mptcp_check_push(sk, ssk);
> +			mptcp_data_unlock(sk);
> +		}
>  		return false;
>  	}

[Severity: Medium]
Should this fallback path also run for active-open subflows and clean up
msk->rtx_queue the same way mptcp_incoming_options() does?

net/mptcp/options.c:mptcp_incoming_options() {
	...
	if (__mptcp_check_fallback(msk)) {
		...
		mptcp_data_lock(subflow->conn);
		if (sk_stream_memory_free(sk))
			__mptcp_check_push(subflow->conn, sk);
		...
		__mptcp_snd_una_update(msk, READ_ONCE(msk->snd_nxt));

		__mptcp_data_acked(subflow->conn);
		mptcp_data_unlock(subflow->conn);
		return !mptcp_over_limit(subflow->conn, sk, skb);
	}
	...
}

Because check_fully_established() returns false, mptcp_incoming_options()
exits without running its fallback block on the packet that triggers the
fallback.

On a passive TFO subflow, __mptcp_check_push() spools pending dfrags to
ssk and advances msk->snd_nxt, but omitting __mptcp_snd_una_update() and
__mptcp_data_acked() leaves those dfrags in msk->rtx_queue until another
packet arrives, keeping the bytes charged to both sk->sk_wmem_queued and
ssk->sk_wmem_queued and double-counting SIOCOUTQ.

On an active-open subflow, msk->wnd_end is initialized from the unscaled
SYN-ACK window, so mptcp_sendmsg() can leave unsent data in msk->rtx_queue
beyond the initial flight.  If a middlebox strips MPTCP options after the
SYN-ACK and the peer sends a bare TCP ACK opening the scaled window,
gating __mptcp_check_push() on syn_fastopen_child skips pushing the
remaining data, and __mptcp_retrans() -> __mptcp_push_retrans() aborts on
__mptcp_check_fallback(msk) without setting MPTCP_PUSH_PENDING, stalling
the remaining data in msk->rtx_queue if the peer is waiting for it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=25