Re: [PATCH mptcp-net 3/7] mptcp: tfo: record 'deny join id0' info

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 06/09/2025 16:05, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for this fix.
> 
> On Fri, 2025-08-29 at 22:33 +0200, Matthieu Baerts (NGI0) wrote:
>> When TFO is used, the check to see if the 'C' flag (deny join id0)
>> was
>> set was bypassed.
>>
>> This flag can be set when TFO is used, so the check should also be
>> done
>> when TFO is used.
>>
>> Fixes: dfc8d0603033 ("mptcp: implement delayed seq generation for
>> passive fastopen")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/options.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index
>> d47b8a9bc2df2f14645b1b3d3e10fea1b38567b1..cf531f2d815cdfbc772b837def6
>> e7d558e64d558 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -985,14 +985,14 @@ static bool check_fully_established(struct
>> mptcp_sock *msk, struct sock *ssk,
>>  		return false;
>>  	}
>>  
>> -	if (mp_opt->deny_join_id0)
>> -		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
>> -
>>  	if (unlikely(!READ_ONCE(msk->pm.server_side)))
>>  		/* DO-NOT-MERGE: use WARN i/o pr_warn: only for
>> MPTCP export */
>>  		WARN_ONCE(1, "bogus mpc option on established client
>> sk");
>>  
>>  set_fully_established:
>> +	if (mp_opt->deny_join_id0)
>> +		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
>> +
> 
> This set_fully_established label is not only jumped to during TFO but
> also in several other scenarios. I think maybe instead of relocating
> the deny_join_id0 check code after this label, we should add a new
> label before the deny_join_id0 check and jump to the new label only in
> the case of TFO.

I don't think that's needed: if I'm not mistaken, the only other
scenario is when a 4th ACK is received. In this case, deny_join_id0 will
not be set.

I don't think we need to add another branch: it is then fine to add this
extra check in the slow path. No?

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