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
| ||
|
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.
Powered by blists - more mailing lists