Re: [PATCH net v2] Fix race for duplicate reqsk on identical SYN

[luoxuanqiang]

在 2024/6/15 14:40, Eric Dumazet 写道:
> On Sat, Jun 15, 2024 at 12:24 AM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
> 你好 Eric和Kuniyuk,
> > From: luoxuanqiang <luoxuanqiang@xxxxxxxxxx>
> > Date: Fri, 14 Jun 2024 20:42:07 +0800
> > > 在 2024/6/14 18:54, Florian Westphal 写道:
> > > > luoxuanqiang <luoxuanqiang@xxxxxxxxxx> wrote:
> > > > >    include/net/inet_connection_sock.h |  2 +-
> > > > >    net/dccp/ipv4.c                    |  2 +-
> > > > >    net/dccp/ipv6.c                    |  2 +-
> > > > >    net/ipv4/inet_connection_sock.c    | 15 +++++++++++----
> > > > >    net/ipv4/tcp_input.c               | 11 ++++++++++-
> > > > >    5 files changed, 24 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
> > > > > index 7d6b1254c92d..8773d161d184 100644
> > > > > --- a/include/net/inet_connection_sock.h
> > > > > +++ b/include/net/inet_connection_sock.h
> > > > > @@ -264,7 +264,7 @@ struct sock *inet_csk_reqsk_queue_add(struct sock *sk,
> > > > >                                   struct request_sock *req,
> > > > >                                   struct sock *child);
> > > > >    void inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
> > > > > -                             unsigned long timeout);
> > > > > +                             unsigned long timeout, bool *found_dup_sk);
> > > > Nit:
> > > >
> > > > I think it would be preferrable to change retval to bool rather than
> > > > bool *found_dup_sk extra arg, so one can do
> > +1
> >
> >
> > > > bool inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
> > > >                                 unsigned long timeout)
> > > > {
> > > >      if (!reqsk_queue_hash_req(req, timeout))
> > > >              return false;
> > > >
> > > > i.e. let retval indicate wheter reqsk was inserted or not.
> > > >
> > > > Patch looks good to me otherwise.
> > > Thank you for your confirmation!
> > >
> > > Regarding your suggestion, I had considered it before,
> > > but besides tcp_conn_request() calling inet_csk_reqsk_queue_hash_add(),
> > > dccp_v4(v6)_conn_request() also calls it. However, there is no
> > > consideration for a failed insertion within that function, so it's
> > > reasonable to let the caller decide whether to check for duplicate
> > > reqsk.
> > I guess you followed 01770a1661657 where found_dup_sk was introduced,
> > but note that the commit is specific to TCP SYN Cookie and TCP Fast Open
> > and DCCP is not related.
> >
> > Then, own_req is common to TCP and DCCP, so found_dup_sk was added as an
> > additional argument.
> >
> > However, another similar commit 5e0724d027f05 actually added own_req check
> > in DCCP path.
> >
> > I personally would'nt care if DCCP was not changed to handle such a
> > failure because DCCP will be removed next year, but I still prefer
> > Florian's suggestion.
> Other things to consider :
>
> - I presume this patch targets net tree, and luoxuanqiang needs the
> fix to reach stable trees.
>
> - This means a Fixes: tag is needed
>
> - This also means that we should favor a patch with no or trivial
> conflicts for stable backports.
>
> Should the patch target the net-next tree, then the requirements can
> be different.

Hi Kuniyuk and Florian,

I've created version 3 based on your suggestions, but I've kept the use
of 'found_dup_sk' since we need to pass NULL in DCCP to maintain its
logic unchanged. Could you please review this update and let me know if
it's okay? Thank you!

BRs!