linux-kernel - 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] [day] [month] [year] [list]

Message-ID: <20230711181634.52860-1-kuniyu@amazon.com>
Date:   Tue, 11 Jul 2023 11:16:34 -0700
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <lmb@...valent.com>
CC:     <andrii@...nel.org>, <ast@...nel.org>, <bpf@...r.kernel.org>,
        <daniel@...earbox.net>, <davem@...emloft.net>,
        <dsahern@...nel.org>, <edumazet@...gle.com>, <haoluo@...gle.com>,
        <hemanthmalla@...il.com>, <joe@...ium.io>, <joe@...d.net.nz>,
        <john.fastabend@...il.com>, <jolsa@...nel.org>,
        <kpsingh@...nel.org>, <kuba@...nel.org>, <kuniyu@...zon.com>,
        <linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
        <martin.lau@...ux.dev>, <mykolal@...com>, <netdev@...r.kernel.org>,
        <pabeni@...hat.com>, <sdf@...gle.com>, <shuah@...nel.org>,
        <song@...nel.org>, <willemdebruijn.kernel@...il.com>, <yhs@...com>
Subject: Re: [PATCH bpf-next v5 6/7] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign

From: Lorenz Bauer <lmb@...valent.com>
Date: Tue, 11 Jul 2023 17:15:06 +0100
> 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?

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!