netdev - Re: [PATCH v2 net] udp: Update reuse->has_conns under reuseport

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] [day] [month] [year] [list]

Message-ID: <20221013184618.77785-1-kuniyu@amazon.com>
Date:   Thu, 13 Oct 2022 11:46:18 -0700
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <martin.lau@...ux.dev>
CC:     <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
        <kraig@...gle.com>, <kuba@...nel.org>, <kuni1840@...il.com>,
        <kuniyu@...zon.com>, <martin.lau@...nel.org>,
        <netdev@...r.kernel.org>, <pabeni@...hat.com>, <willemb@...gle.com>
Subject: Re: [PATCH v2 net] udp: Update reuse->has_conns under reuseport_lock.

From:   Martin KaFai Lau <martin.lau@...ux.dev>
Date:   Thu, 13 Oct 2022 10:41:53 -0700
> On 10/13/22 9:09 AM, Eric Dumazet wrote:
> >>>> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> >>>> index 5daa1fa54249..abb414ed4aa7 100644
> >>>> --- a/net/core/sock_reuseport.c
> >>>> +++ b/net/core/sock_reuseport.c
> >>>> @@ -21,6 +21,21 @@ static DEFINE_IDA(reuseport_ida);
> >>>>   static int reuseport_resurrect(struct sock *sk, struct sock_reuseport *old_reuse,
> >>>>                                 struct sock_reuseport *reuse, bool bind_inany);
> >>>>
> >>>> +void reuseport_has_conns_set(struct sock *sk)
> >>>> +{
> >>>> +       struct sock_reuseport *reuse;
> >>>> +
> >>>> +       if (!rcu_access_pointer(sk->sk_reuseport_cb))
> >>>> +               return;
> >>>> +
> >>>> +       spin_lock(&reuseport_lock);
> 
> It seems other paths are still using the spin_lock_bh().  It will be useful to 
> have a few words here why _bh() is not needed.

I think I forgot to add _bh(), but I'm now wondering what is the hlist
lock mentioned in reuseport_alloc()...


> >>>> +       reuse = rcu_dereference_protected(sk->sk_reuseport_cb,
> >>>> +                                         lockdep_is_held(&reuseport_lock));
> >>>
> >>> Could @reuse be NULL at this point ?
> >>>
> >>> Previous  test was performed without reuseport_lock being held.
> >>
> >> Usually, sk_reuseport_cb is changed under lock_sock().
> >>
> >> The only exception is reuseport_grow() & TCP reqsk migration case.
> >>
> >> 1) shutdown() TCP listener, which is moved into the latter part of
> >>     reuse->socks[] to migrate reqsk.
> >>
> >> 2) New listen() overflows reuse->socks[] and call reuseport_grow().
> >>
> >> 3) reuse->max_socks overflows u16 with the new listener.
> >>
> >> 4) reuseport_grow() pops the old shutdown()ed listener from the array
> >>     and update its sk->sk_reuseport_cb as NULL without lock_sock().
> >>
> >> shutdown()ed sk->sk_reuseport_cb can be changed without lock_sock().
> >>
> >> But, reuseport_has_conns_set() is called only for UDP and under
> >> lock_sock(), so @reuse never be NULL in this case.
> > 
> > Given the complexity of this code and how much time is needed to
> > review all possibilities, please add an additional
> > 
> > if (reuse)
> >     reuse->has_conns = 1;
> 
> +1

Acked.

Thank you.