Re: [PATCH mptcp-net] mptcp: fallback earlier on simult connection

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

On 28/11/2025 13:22, Paolo Abeni wrote:
> Syzkaller reports a simult-connect race leading to inconsistent fallback
> status:

(...)

> The TCP subflow can process the simult-connect syn-ack packet after
> transitioning to TCP_FIN1 state, bypassing the MPTCP fallback check,
> as the sk_state_change() callback is not invoked for * -> FIN_WAIT1
> transitions.
> 
> That will move the msk socket to an inconsistent status and the next
> incoming data will hit the reported splat.
> 
> Close the race moving the simult-fallback check at the earliest possible
> stage - that is at syn-ack generation time.

Good idea!

> Fixes: 23e89e8ee7be ("tcp: Don't drop SYN+ACK for simultaneous connect().")

From what I understand, the modification on TCP side is needed for this
fix. When reading its commit message, it sounds it should have contained
a Fixes tag, but net-next was chosen to delay the fix. Is that correct?

If yes, should I ask to backport this other commit with this patch? Or
should this patch be backported only up to stable trees already having
this other commit?

> Fixes: 4fd19a307016 ("mptcp: fix inconsistent state on fastopen race")
> Fixes: 1e777f39b4d7 ("mptcp: add MSG_FASTOPEN sendmsg flag support")> Reported-by: syzbot+0ff6b771b4f7a5bce83b@syzkaller.appspotmail.com
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/586
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> @mattbe: could you please test vs yours syz repro?

In progress!
> simult connect pkt drill test will need a paired change; not reporting
> the full diff to avoid confusing the patch importer:
> 
>  +0  >  S  0:0(0)                    <mss 1460, sackOK, TS val 100 ecr 0,   nop, wscale 8, mpcapable v1 flags[flag_h] nokey>
>  +0  <  S  0:0(0)         win 1000   <mss 1460, sackOK, TS val 407 ecr 0,   nop, wscale 8, mpcapable v1 flags[flag_h] nokey>
> -+0  >  S. 0:0(0)  ack 1             <mss 1460, sackOK, TS val 330 ecr 407, nop, wscale 8, mpcapable v1 flags[flag_h] nokey>
> -+0  <  S. 0:0(0)  ack 1  win 65535  <mss 1460, sackOK, TS val 507 ecr 100, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey=2]>
> ++0  >  S. 0:0(0)  ack 1             <mss 1460, sackOK, TS val 330 ecr 407, nop,wscale 8>
> ++0  <  S. 0:0(0)  ack 1  win 65535  <mss 1460, sackOK, TS val 507 ecr 100, nop,wscale 8>
>  +0  >   . 1:1(0)  ack 1             <nop,      nop,    TS val 430 ecr 507, nop, nop, sack 0:1>

Thank you! Confirmed!

(...)

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index bc470254bd6b..b995b009f31d 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1325,19 +1325,12 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
>  	return false;
>  }
>  
> -static inline bool is_active_ssk(struct mptcp_subflow_context *subflow)
> -{
> -	return (subflow->request_mptcp || subflow->request_join);

Out of curiosity, was this "subflow->request_join" not there to catch
simultaneous MPJoin connect?

Is it not needed to catch this case, and drop the second join?

https://datatracker.ietf.org/doc/html/rfc8684#section-3.9.2-8

(Or maybe the TCP code handles that as mentioned in the RFC?)

> -}
> -
>  static inline bool subflow_simultaneous_connect(struct sock *sk)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>  
> -	return (1 << sk->sk_state) &
> -	       (TCPF_ESTABLISHED | TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING) &&
> -	       is_active_ssk(subflow) &&
> -	       !subflow->conn_finished;
> +	/* Note that the sk state implies !subflow->conn_finished. */
> +	return sk->sk_state == TCP_SYN_RECV && subflow->request_mptcp;

Detail: do we still require this helper? It is only used in one place,
already under "subflow->request_mptcp". But we can keep it if it is
easier / clearer.

>  }
>  
>  #ifdef CONFIG_SYN_COOKIES
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 86ce58ae533d..96d54cb2cd93 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1878,12 +1878,6 @@ static void subflow_state_change(struct sock *sk)
>  
>  	__subflow_state_change(sk);
>  
> -	if (subflow_simultaneous_connect(sk)) {
> -		WARN_ON_ONCE(!mptcp_try_fallback(sk, MPTCP_MIB_SIMULTCONNFALLBACK));
> -		subflow->conn_finished = 1;
> -		mptcp_propagate_state(parent, sk, subflow, NULL);
> -	}
> -
>  	/* 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.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.