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;
MIME-Version: 1.0
References: <20230613-so-reuseport-v5-0-f6686a0dbce0@isovalent.com> <20230613-so-reuseport-v5-6-f6686a0dbce0@isovalent.com>
In-Reply-To: <20230613-so-reuseport-v5-6-f6686a0dbce0@isovalent.com>
From:   Lorenz Bauer <lmb@isovalent.com>
Date:   Tue, 11 Jul 2023 17:15:06 +0100
Message-ID: <CAN+4W8gs84r+PVWgMbic29Opj2EviNMh7AzcP=BR3CLvYHiQWg@mail.gmail.com>
Subject: Re: [PATCH bpf-next v5 6/7] bpf, net: Support SO_REUSEPORT sockets
 with bpf_sk_assign
To:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        David Ahern <dsahern@kernel.org>,
        Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Joe Stringer <joe@wand.net.nz>,
        Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>,
        Kuniyuki Iwashima <kuniyu@amazon.com>
Cc:     Hemanth Malla <hemanthmalla@gmail.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
        linux-kselftest@vger.kernel.org, Joe Stringer <joe@cilium.io>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jul 4, 2023 at 2:46=E2=80=AFPM Lorenz Bauer <lmb@isovalent.com> wro=
te:
>
> +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 =3D skb_steal_sock(skb, refcounted, &prefetched);
> +       if (!sk)
> +               return NULL;
> +
> +       if (!prefetched)
> +               return sk;
> +
> +       if (sk->sk_protocol =3D=3D IPPROTO_TCP) {
> +               if (sk->sk_state !=3D TCP_LISTEN)
> +                       return sk;
> +       } else if (sk->sk_protocol =3D=3D IPPROTO_UDP) {
> +               if (sk->sk_state !=3D TCP_CLOSE)
> +                       return sk;
> +       } else {
> +               return sk;
> +       }
> +
> +       reuse_sk =3D inet6_lookup_reuseport(net, sk, skb, doff,
> +                                         saddr, sport, daddr, ntohs(dpor=
t),
> +                                         ehashfn);
> +       if (!reuse_sk)
> +               return sk;
> +
> +       /* We've chosen a new reuseport sock which is never refcounted. T=
his
> +        * 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