1. Patchew
  2. MPTCP
  3. [PATCH mptcp-net 0/7] mptcp: pm: nl: announce deny-join-id0 attribute
  4. View patch

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

Matthieu Baerts (NGI0) posted 7 patches 2 weeks, 4 days ago
There is a newer version of this series
[PATCH mptcp-net 3/7] mptcp: tfo: record 'deny join id0' info
Posted by Matthieu Baerts (NGI0) 2 weeks, 4 days ago 
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..cf531f2d815cdfbc772b837def6e7d558e64d558 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);
+
 	mptcp_data_lock((struct sock *)msk);
 	__mptcp_subflow_fully_established(msk, subflow, mp_opt);
 	mptcp_data_unlock((struct sock *)msk);

-- 
2.50.1
Re: [PATCH mptcp-net 3/7] mptcp: tfo: record 'deny join id0' info
Posted by Geliang Tang 1 week, 4 days ago 
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.

WDYT?

Thanks,
-Geliang

>  	mptcp_data_lock((struct sock *)msk);
>  	__mptcp_subflow_fully_established(msk, subflow, mp_opt);
>  	mptcp_data_unlock((struct sock *)msk);
Re: [PATCH mptcp-net 3/7] mptcp: tfo: record 'deny join id0' info
Posted by Matthieu Baerts 1 week, 4 days ago 
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.
Re: [PATCH mptcp-net 3/7] mptcp: tfo: record 'deny join id0' info
Posted by Geliang Tang 1 week ago 
On Sat, 2025-09-06 at 16:19 +0200, Matthieu Baerts wrote:
> 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..cf531f2d815cdfbc772b837
> > > def6
> > > 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?

Sure, thanks for your explanation.

-Geliang

> 
> Cheers,
> Matt

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.