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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: data race on cb_flags due to mixed atomic and non-atomic bitops
- [Medium] mptcp: unintended worker scheduling on all TCP state changes for TFO subflows
--

commit ae556f46010534274aa7940f77f235a6807ae115
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, if the server application writes
data while the passive subflow is still in SYN_RECV, the data is queued.
This patch attempts to push stranded data when the underlying subflow
transitions to established by modifying cb_flags and scheduling the MPTCP
worker.

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

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index f0a6725d2c376..f499073a62d7c 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
[ ... ]
> @@ -1894,6 +1894,11 @@ static void subflow_state_change(struct sock *sk)
>  	if (subflow->resetting)
>  		return;
>  
> +	if (subflow->is_mptfo) {

[Severity: Medium]
Since subflow->is_mptfo is never cleared and there is no check for
sk->sk_state == TCP_ESTABLISHED here, won't this trigger on every subsequent
TCP state change?

The commit message states this logic is intended specifically for when "the
underlying subflow transitions to TCP_ESTABLISHED", but without checking the
state, this could fire erroneously and schedule the worker when transitioning
into shutdown states like TCP_FIN_WAIT1, TCP_CLOSE_WAIT, or TCP_CLOSE.

> +		set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(parent)->cb_flags);

[Severity: High]
Does mixing atomic and non-atomic bit operations on cb_flags cause data races
and lost updates?

subflow_state_change() uses the atomic set_bit() operation to modify
cb_flags without holding the parent MPTCP socket's mptcp_data_lock.

However, concurrently, other paths modify cb_flags using non-atomic operations
while holding mptcp_data_lock. For example:

net/mptcp/protocol.c:__mptcp_check_push() {
    ...
    } else {
        __set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
    }
}

If the atomic set_bit() occurs between the load and store of the non-atomic
operation, the newly set bit will be clobbered and permanently lost, which
could lead to stranded data or missing wakeups.

> +		mptcp_schedule_work(parent);
> +	}
> +
>  	/* as recvmsg() does not acquire the subflow socket for ssk selection

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008095231.103186-1-rameshkumar.t@phytecembedded.in?part=1