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

T S Rameshkumar posted 1 patch 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20261008121602.206507-1-rameshkumar.t@phytecembedded.in
There is a newer version of this series
net/mptcp/options.c | 9 +++++++++
1 file changed, 9 insertions(+)
[PATCH v4] mptcp: push queued data on passive TFO subflows becoming established
Posted by T S Rameshkumar 1 week ago
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")
Assisted-by: Gemini 3.7
Signed-off-by: T S Rameshkumar <rameshkumar.t@phytecembedded.in>
---
 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 v4] mptcp: push queued data on passive TFO subflows becoming established
Posted by MPTCP CI 1 week ago
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/37777427552

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


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)
Re: [PATCH v4] mptcp: push queued data on passive TFO subflows becoming established
Posted by sashiko-bot@kernel.org 1 week ago
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