Re: [PATCH export v3 2/4] mptcp: reject joins after fallback in mptcp_is_fully_established

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

Thank you for your reply!

14 Aug 2026 10:43:41 Paolo Abeni <pabeni@redhat.com>:

>
>
> On 8/12/26 5:32 PM, Matthieu Baerts wrote:
>> Hi Paolo, Chenguang,
>>
>> On 12/08/2026 11:45, Paolo Abeni wrote:
>>> On 8/12/26 7:46 AM, Chenguang Zhao wrote:
>>>> From: Chenguang Zhao <zhaochenguang@kylinos.cn>
>>>>
>>>> After fallback, treat the connection as not fully established so later
>>>> MP_JOIN attempts are rejected.
>>
>> @Chenguang: please first reply to the questions and comments from the
>> previous versions, then only send a new version when the discussions are
>> over.
>>
>>>> Signed-off-by: Chenguang Zhao <zhaochenguang@kylinos.cn>
>>>> ---
>>>> net/mptcp/protocol.h | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>> index 7e168e450fb0..bf2483a7ed92 100644
>>>> --- a/net/mptcp/protocol.h
>>>> +++ b/net/mptcp/protocol.h
>>>> @@ -957,8 +957,11 @@ static inline void mptcp_start_tout_timer(struct sock *sk)
>>>>
>>>> static inline bool mptcp_is_fully_established(struct sock *sk)
>>>> {
>>>> +   struct mptcp_sock *msk = mptcp_sk(sk);
>>>> +
>>>>     return inet_sk_state_load(sk) == TCP_ESTABLISHED &&
>>>> -          READ_ONCE(mptcp_sk(sk)->fully_established);
>>>> +          READ_ONCE(msk->fully_established) &&
>>>> +          !test_bit(MPTCP_FALLBACK_DONE, &msk->flags);
>>>
>>> Does the above improve actually anything? The test is inherently racy,
>>> as lack the fallback_lock, and AFAICS all critical paths have already
>>> explicit checks under such lock, see i.e.  mptcp_finish_join().
>>
>> Indeed, the fallback lock is missing.
>>
>> Note: Chenguang is looking at a bug where, when an MP_FAIL is received
>> before being in fully_established, the kernel accept going to fully
>> established, and creating new subflows, then reject them once done.
>>
>> Same when receiving a 4th packet without MPTCP options.
>
> It's not clear to me what actually goes wrong and how. Do you have handy
> a event sequence or stack trace demonstrating the critical scenario?

Indeed, better with packetdrill scripts:

https://github.com/chrisocean716-star/packetdrill/blob/bf9be7a7bee3687500444b284a9db2a0009a5252/gtests/net/mptcp/dss/dss_drop_after_data_fallback_server.pkt

=> A fallback is done, a subflow can be initiated, then rejected.


https://github.com/chrisocean716-star/packetdrill/blob/fed57fab92c407eb433acdb8c8ce72bd8e3e2f37/gtests/net/mptcp/mp_join/mp_join_server_after_fallback.pkt

=> The MP_FAIL is partially ignored if received before being in fully
established state.

> Both the above look like bugs in check_fully_established(), or
> incorrect/later check of fallback status/fallback_lock.
>
> i.e. AFAICS active subflows are created and added to the conn_list
> without checking the fallback status/fallback_lock, but the later join
> correctly rejects the subflow finalization, so there should any real
> problems beyond some noise.

For the fallback, it feels wrong but we could say that it's just some noise.

For the MP_FAIL, we should probably not ignore it in this case.

I think the MP_FAIL case should fallback. For both, the fallback should
either disable the fully established state, or the fully established
helpers should look for fallback.

Cheers,
Matt