Re: [PATCH net 1/4] mptcp: reset msk state on early connect failure

[Matthieu Baerts <matttbe@kernel.org>]

Hello,

On 14/10/2026 16:32, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> 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).
> 
> - [Medium] The new error path in mptcp_connect()
>   (net/mptcp/protocol.c:4224) calls…

TL;DR: this is a pre-existing issue.

> mptcp: reset msk state on early connect failure
> 
> This commit changes the mptcp_connect() error path. It now clears the
> fallback state (MPTCP_FALLBACK_DONE, allow_subflows,
> allow_infinite_fallback) and resets the first subflow context. The goal
> is that a later connect() on the same fd can use MPTCP again after an
> early fallback followed by an immediate connect failure.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/633
> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index e89a69ab927c9..e6f73a5ee630a 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -4212,9 +4212,16 @@ static int mptcp_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
>>  	 * subflow_finish_connect()
>>  	 */
>>  	if (unlikely(err)) {
>> -		/* avoid leaving a dangling token in an unconnected socket */
>>  		mptcp_token_destroy(msk);
>>  		mptcp_set_state(sk, TCP_CLOSE);
>> +
>> +		spin_lock_bh(&msk->fallback_lock);
>> +		msk->allow_subflows = true;
>> +		msk->allow_infinite_fallback = true;
>> +		clear_bit(MPTCP_FALLBACK_DONE, &msk->flags);
>> +		spin_unlock_bh(&msk->fallback_lock);
>> +
>> +		mptcp_subflow_ctx_reset(mptcp_subflow_ctx(ssk));
> 
> [Severity: Medium]
> Is mptcp_subflow_ctx_reset() resetting more than this path needs? It
> clears the whole reset group and then sets local_id to -1:
> 
> net/mptcp/protocol.h:mptcp_subflow_ctx_reset() {
> 	memset(&subflow->reset, 0, sizeof(subflow->reset));
> 	subflow->request_mptcp = 1;
> 	WRITE_ONCE(subflow->local_id, -1);
> }
> 
> This also wipes two values that are set only once, when msk->first is
> created:
> 
> net/mptcp/protocol.c:__mptcp_socket_create() {
>     ...
> 	subflow->subflow_id = msk->subflow_id++;
> 
> 	/* This is the first subflow, always with id 0 */
> 	WRITE_ONCE(subflow->local_id, 0);
>     ...
> }
> 
> A second connect() gets the same msk->first back from __mptcp_nmpc_sk(),
> so this setup does not run again. The reset also runs on every immediate
> failure (ENETUNREACH, EAFNOSUPPORT, BPF pre_connect, ...), not only after
> an early fallback.
> 
> On the retry, local_id is -1, so the connect goes through:
> 
> tcp_connect()
>   subflow_rebuild_header()
>     subflow_chk_local_id()
>       mptcp_pm_get_local_id()
> 
> mptcp_pm_get_local_id() returns 0 only when the msk local address matches
> the subflow's address:
> 
> 	mptcp_local_address((struct sock_common *)msk, &msk_local);
> 	mptcp_local_address((struct sock_common *)skc, &skc_local.addr);
> 	if (mptcp_addresses_equal(&msk_local, &skc_local.addr, false))
> 		return 0;
> 
> mptcp_copy_inaddrs() runs only after a successful connect. For an unbound
> msk, the msk address is therefore still zero at this point.
> 
> Wouldn't the kernel PM then give the MPC subflow a non-zero ID, either
> from a matching endpoint or by adding an IMPLICIT endpoint to the netns
> endp_list?
> 
> If the endpoint table is full, mptcp_pm_nl_get_local_id() returns -ERANGE.
> tcp_connect() would then fail the retry with -EHOSTUNREACH, even though a
> valid route exists.
> 
> A non-zero local_id on the initial subflow also seems to break endpoint
> removal. When that endpoint is deleted,
> mptcp_nl_remove_subflow_and_signal_addr() gets the ID from
> mptcp_endp_get_local_id(), which returns rm_id 0 for the MPC endpoint.
> mptcp_pm_rm_addr_or_subflow() then skips the initial subflow because
> id != rm_id, so the initial subflow is never closed.
> 
> The reset also zeroes subflow_id, while msk->subflow_id has already
> advanced to 2. After a successful retry, mptcp_get_full_info() would
> report the first subflow with id 0:
> 
> 	sfinfo.id = subflow->subflow_id;
> 
> But tools/testing/selftests/net/mptcp/mptcp_sockopt.c expects:
> 
> 	assert(sfinfo[0].id == 1);
> 
> mptcp_disconnect() already behaves this way for subflow_id. The local_id
> part looks new, though: on the disconnect path, the msk still holds the
> addresses from the earlier successful connect.

The code here is doing a "disconnect" of the first subflow: similar to
what is done in __mptcp_subflow_disconnect(), mptcp_subflow_ctx_reset()
is called. It will indeed set subflow->local_id to -1 and causing the
(minor) issue reported here where the PM could assign the wrong ID when
reconnecting (yeah, another reconnect after a "disconnect" case...). So
I don't think this issue is new, and the fix here is doing the right
thing of calling mptcp_subflow_ctx_reset().

> The commit message mentions only MPTCP_FALLBACK_DONE, allow_subflows and
> request_mptcp. Would it work to reset only request_mptcp here (plus the
> token and key fields if needed)? Another option would be to restore
> local_id to 0 and keep the original subflow_id for msk->first after
> mptcp_subflow_ctx_reset().

I don't think we should set the local_id to 0 in mptcp_subflow_ctx_reset
because it will cause issues in mptcp_close_ssk(). Maybe we should set
local_id to 0 for the first subflow in mptcp_disconnect()? Anyway, that
should be part of a follow-up patch (I can look at that), not as a new
version of this one.

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