Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kuniyuki Iwashima <kuniyu@amazon.com>
To:     <pabeni@redhat.com>
CC:     <davem@davemloft.net>, <dsahern@kernel.org>, <edumazet@google.com>,
        <kraig@google.com>, <kuba@kernel.org>, <kuni1840@gmail.com>,
        <kuniyu@amazon.com>, <linux-kernel@vger.kernel.org>,
        <martin.lau@kernel.org>, <netdev@vger.kernel.org>,
        <willemb@google.com>, <yoshfuji@linux-ipv6.org>
Subject: Re: [PATCH v1 net 1/3] udp: Update reuse->has_conns under reuseport_lock.
Date:   Tue, 11 Oct 2022 08:16:18 -0700
Message-ID: <20221011151618.89550-1-kuniyu@amazon.com>
In-Reply-To: <468f01fc1dde6cf44fab51653eeb626fc8521db2.camel@redhat.com>
References: <468f01fc1dde6cf44fab51653eeb626fc8521db2.camel@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk

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