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

[Geliang Tang <geliang@kernel.org>]

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