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

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?

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.

Thanks

Lorenz