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

Hi Chenguang,

On 13/08/2026 10:45, Chenguang Zhao wrote:
> 
> 在 2026/8/12 17:45, Paolo Abeni 写道:
>> On 8/12/26 7:46 AM, Chenguang Zhao wrote:
>>> From: Chenguang Zhao <[email protected]>
>>>
>>> After fallback, treat the connection as not fully established so later
>>> MP_JOIN attempts are rejected.
>>>
>>> Signed-off-by: Chenguang Zhao <[email protected]>
>>> ---
>>>  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().
>>
>> I would prefer avoiding additional conditionals, if not well reasoned.
>>
>> /P
>>
> Hi Paolo
> 
> You are right: the extra FALLBACK_DONE check in
> mptcp_is_fully_established() does not actually improve anything.
> It is read without fallback_lock, so it is racy. The paths that must
> not complete a join after MP_FAIL already reject it under that lock
> via allow_subflows, e.g. mptcp_finish_join() / __mptcp_finish_join().
> I will drop this patch in the next revision and leave
> mptcp_is_fully_established() unchanged.

How do you plan to deal with the case where a fallback has been done
while being in fully established mode?

Could we add WRITE_ONCE(msk->fully_established, false) in
__mptcp_try_fallback()? I guess it is acceptable because
__mptcp_finish_join will check allow_subflows under lock, no?

Or maybe we accept this behaviour?

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