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

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAN+4W8gs84r+PVWgMbic29Opj2EviNMh7AzcP=BR3CLvYHiQWg@mail.gmail.com>
Date: Tue, 11 Jul 2023 17:15:06 +0100
From: Lorenz Bauer <lmb@...valent.com>
To: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>, 
	Willem de Bruijn <willemdebruijn.kernel@...il.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>, 
	John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	Joe Stringer <joe@...d.net.nz>, Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: Hemanth Malla <hemanthmalla@...il.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, Joe Stringer <joe@...ium.io>
Subject: 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@...valent.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