Re: [PATCH v1 net 1/3] udp: Update reuse->has_conns under reuseport_lock.

From: Kuniyuki Iwashima
Date: Tue Oct 11 2022 - 11:27:32 EST


From: Paolo Abeni <pabeni@xxxxxxxxxx>
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@xxxxxxxxxx>
> > ---
> > 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.