netdev - Re: [PATCH net] tcp/dccp: fix possible race __inet_lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Dec 2019 09:43:15 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     netdev <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Firo Yang <firo.yang@...e.com>
Subject: Re: [PATCH net] tcp/dccp: fix possible race __inet_lookup_established()

On Thu, Dec 12, 2019 at 9:32 AM Michal Kubecek <mkubecek@...e.cz> wrote:
>
> On Wed, Dec 11, 2019 at 09:09:43AM -0800, Eric Dumazet wrote:
> > Michal Kubecek and Firo Yang did a very nice analysis of crashes
> > happening in __inet_lookup_established().
> >
> > Since a TCP socket can go from TCP_ESTABLISH to TCP_LISTEN
> > (via a close()/socket()/listen() cycle) without a RCU grace period,
> > I should not have changed listeners linkage in their hash table.
> >
> > They must use the nulls protocol (Documentation/RCU/rculist_nulls.txt),
> > so that a lookup can detect a socket in a hash list was moved in
> > another one.
> >
> > Since we added code in commit d296ba60d8e2 ("soreuseport: Resolve
> > merge conflict for v4/v6 ordering fix"), we have to add
> > hlist_nulls_add_tail_rcu() helper.
> >
> > Fixes: 3b24d854cb35 ("tcp/dccp: do not touch listener sk_refcnt under synflood")
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > Reported-by: Michal Kubecek <mkubecek@...e.cz>
> > Reported-by: Firo Yang <firo.yang@...e.com>
> > Link: https://lore.kernel.org/netdev/20191120083919.GH27852@unicorn.suse.cz/
> > ---
> >  include/linux/rculist_nulls.h | 37 +++++++++++++++++++++++++++++++++++
> >  include/net/inet_hashtables.h | 11 +++++++++--
> >  include/net/sock.h            |  5 +++++
> >  net/ipv4/inet_diag.c          |  3 ++-
> >  net/ipv4/inet_hashtables.c    | 16 +++++++--------
> >  net/ipv4/tcp_ipv4.c           |  7 ++++---
> >  6 files changed, 65 insertions(+), 14 deletions(-)
> >
> [...]
> > diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> > index af2b4c065a042e36135fe6fdcee9833b6b353364..29ef5b7f4005a8e67fd358c136ee6532974efcab 100644
> > --- a/include/net/inet_hashtables.h
> > +++ b/include/net/inet_hashtables.h
> > @@ -105,11 +105,18 @@ struct inet_bind_hashbucket {
> >
> >  /*
> >   * Sockets can be hashed in established or listening table
> > - */
> > + * We must use different 'nulls' end-of-chain value for listening
> > + * hash table, or we might find a socket that was closed and
> > + * reallocated/inserted into established hash table
> > +  */
>
> Just a nitpick: I don't think this comment is still valid because
> listening sockets now have RCU protection so that listening socket
> cannot be freed and reallocated without RCU grace period. (But we still
> need disjoint ranges to handle the reallocation in the opposite
> direction.)

Hi Michal

I am not a native English speaker, but I was trying to say :

A lookup in established sockets might go through a socket that
was in this bucket but has been closed, reallocated and became a listener.

Maybe the comment needs to be refined, but I am not sure how, considering
that most people reading it will not understand it anyway, given the
complexity of the nulls stuff.

>
> Other than that, the patch looks good (and better than my
> work-in-progress patch which I didn't manage to test properly).
>
> Reviewed-by: Michal Kubecek <mkubecek@...e.cz>
>
> > +#define LISTENING_NULLS_BASE (1U << 29)
> >  struct inet_listen_hashbucket {
> >       spinlock_t              lock;
> >       unsigned int            count;
> > -     struct hlist_head       head;
> > +     union {
> > +             struct hlist_head       head;
> > +             struct hlist_nulls_head nulls_head;
> > +     };
> >  };