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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Oct 2022 08:16:18 -0700 From: Kuniyuki Iwashima <kuniyu@...zon.com> To: <pabeni@...hat.com> CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>, <kraig@...gle.com>, <kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>, <linux-kernel@...r.kernel.org>, <martin.lau@...nel.org>, <netdev@...r.kernel.org>, <willemb@...gle.com>, <yoshfuji@...ux-ipv6.org> Subject: Re: [PATCH v1 net 1/3] udp: Update reuse->has_conns under reuseport_lock. From: Paolo Abeni <pabeni@...hat.com> Date: Tue, 11 Oct 2022 12:50:52 +0200 > On Mon, 2022-10-10 at 10:43 -0700, Kuniyuki Iwashima wrote: > > When we call connect() for a UDP socket in a reuseport group, we have > > to update sk->sk_reuseport_cb->has_conns to 1. Otherwise, the kernel > > could select a unconnected socket wrongly for packets sent to the > > connected socket. > > > > However, the current way to set has_conns is illegal and possible to > > trigger that problem. reuseport_has_conns() changes has_conns under > > rcu_read_lock(), which upgrades the RCU reader to the updater. Then, > > it must do the update under the updater's lock, reuseport_lock, but > > it doesn't for now. > > > > For this reason, there is a race below where we fail to set has_conns > > resulting in the wrong socket selection. To avoid the race, let's split > > the reader and updater with proper locking. > > > > cpu1 cpu2 > > +----+ +----+ > > > > __ip[46]_datagram_connect() reuseport_grow() > > . . > > > - reuseport_has_conns(sk, true) |- more_reuse = __reuseport_alloc(more_socks_size) > > > . | > > > |- rcu_read_lock() > > > |- reuse = rcu_dereference(sk->sk_reuseport_cb) > > > | > > > | | /* reuse->has_conns == 0 here */ > > > | |- more_reuse->has_conns = reuse->has_conns > > > |- reuse->has_conns = 1 | /* more_reuse->has_conns SHOULD BE 1 HERE */ > > > | | > > > | |- rcu_assign_pointer(reuse->socks[i]->sk_reuseport_cb, > > > | | more_reuse) > > > `- rcu_read_unlock() `- kfree_rcu(reuse, rcu) > > > > > > - sk->sk_state = TCP_ESTABLISHED > > > > Fixes: acdcecc61285 ("udp: correct reuseport selection with connected sockets") > > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com> > > --- > > include/net/sock_reuseport.h | 23 +++++++++++++++++------ > > net/ipv4/datagram.c | 2 +- > > net/ipv4/udp.c | 2 +- > > net/ipv6/datagram.c | 2 +- > > net/ipv6/udp.c | 2 +- > > 5 files changed, 21 insertions(+), 10 deletions(-) > > > > diff --git a/include/net/sock_reuseport.h b/include/net/sock_reuseport.h > > index 473b0b0fa4ab..fe9779e6d90f 100644 > > --- a/include/net/sock_reuseport.h > > +++ b/include/net/sock_reuseport.h > > @@ -43,21 +43,32 @@ struct sock *reuseport_migrate_sock(struct sock *sk, > > extern int reuseport_attach_prog(struct sock *sk, struct bpf_prog *prog); > > extern int reuseport_detach_prog(struct sock *sk); > > > > -static inline bool reuseport_has_conns(struct sock *sk, bool set) > > +static inline bool reuseport_has_conns(struct sock *sk) > > { > > struct sock_reuseport *reuse; > > bool ret = false; > > > > rcu_read_lock(); > > reuse = rcu_dereference(sk->sk_reuseport_cb); > > - if (reuse) { > > - if (set) > > - reuse->has_conns = 1; > > - ret = reuse->has_conns; > > - } > > + if (reuse && reuse->has_conns) > > + ret = true; > > rcu_read_unlock(); > > > > return ret; > > } > > > > +static inline 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); > > + reuse = rcu_dereference_protected(sk->sk_reuseport_cb, > > + lockdep_is_held(&reuseport_lock)); > > + reuse->has_conns = 1; > > + spin_unlock(&reuseport_lock); > > +} > > Since the above is not super critical, it's probably better move it > into sock_reuseport.c file and export it (to fix the build issue) I'll fix the CONFIG_IPV6=m build failure. Thank you.
Powered by blists - more mailing lists