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

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 <[email protected]>
>>>
>>> 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 <[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().
> 
> 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?

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.

/P