Re: [PATCH bpf-next v5 6/7] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Lorenz Bauer <lmb@isovalent.com>
Date: Tue, 11 Jul 2023 17:15:06 +0100
> On Tue, Jul 4, 2023 at 2:46 PM Lorenz Bauer <lmb@isovalent.com> wrote:
> >
> > +static inline
> > +struct sock *inet6_steal_sock(struct net *net, struct sk_buff *skb, int doff,
> > +                             const struct in6_addr *saddr, const __be16 sport,
> > +                             const struct in6_addr *daddr, const __be16 dport,
> > +                             bool *refcounted, inet6_ehashfn_t *ehashfn)
> > +{
> > +       struct sock *sk, *reuse_sk;
> > +       bool prefetched;
> > +
> > +       sk = skb_steal_sock(skb, refcounted, &prefetched);
> > +       if (!sk)
> > +               return NULL;
> > +
> > +       if (!prefetched)
> > +               return sk;
> > +
> > +       if (sk->sk_protocol == IPPROTO_TCP) {
> > +               if (sk->sk_state != TCP_LISTEN)
> > +                       return sk;
> > +       } else if (sk->sk_protocol == IPPROTO_UDP) {
> > +               if (sk->sk_state != TCP_CLOSE)
> > +                       return sk;
> > +       } else {
> > +               return sk;
> > +       }
> > +
> > +       reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
> > +                                         saddr, sport, daddr, ntohs(dport),
> > +                                         ehashfn);
> > +       if (!reuse_sk)
> > +               return sk;
> > +
> > +       /* We've chosen a new reuseport sock which is never refcounted. This
> > +        * implies that sk also isn't refcounted.
> > +        */
> > +       WARN_ON_ONCE(*refcounted);
> > +
> > +       return reuse_sk;
> > +}
> 
> Hi Kuniyuki,
> 
> Continuing the conversation from v5 of the patch set, you wrote:
> 
> In inet6?_steal_sock(), we call inet6?_lookup_reuseport() only for
> sk that was a TCP listener or UDP non-connected socket until just before
> the sk_state checks.  Then, we know *refcounted should be false for such
> sockets even before inet6?_lookup_reuseport().
> 
> This makes sense for me in the TCP listener case. I understand UDP
> less, so I'll have to rely on your input. I tried to convince myself
> that all UDP sockets in TCP_CLOSE have SOCK_RCU_FREE set. However, the
> only place I see sock_set_flag(sk, SOCK_RCU_FREE) in the UDP case is
> in udp_lib_get_port(). That in turn seems to be called during bind.
> So, what if BPF does bpf_sk_assign() of an unbound and unconnected
> socket?  Wouldn't that trigger the warning?

Ah sorry, I assumed it would not happen, but if we can put unbound
TCP/UDP socket into a map and select it, then yes, it hits the warning.

Let's say we can select a non-RCU sk in bpf_sk_assign() and then the
socket is converted to RCU by bind(udp_sk) or listen(tcp_sk).

The sk_is_refcounted() in bpf_sk_assign() returns true and sk_refcnt
is incremented.  Then, I think of two scenarios:

  1) RCU conversion is done before sk_is_refcounted() in skb_steal_sock().
     -> *refcounted is false

  2) RCU conversion is done after skb_steal_sock().
     -> *refcounted is true

In both cases, we need to decrement the refcnt that is bumped up
by bpf_sk_assign().  The sock_put() in the v1 series does not catch
the former case.

How should we track it ?


> 
> To maybe sidestep this question: do you think the location of the
> WARN_ON_ONCE has to prevent this patch set from going in? I've been
> noodling at it for quite a while already and it would be good to see
> it land.

If the issue above happened, I think it could be a blocker.

Thanks!