Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220607013844.213446-1-jmaxwell37@gmail.com> <165458990767.3884.13816861537144030058@kwain>
In-Reply-To: <165458990767.3884.13816861537144030058@kwain>
From:   Jonathan Maxwell <jmaxwell37@gmail.com>
Date:   Wed, 8 Jun 2022 08:38:34 +1000
Message-ID: <CAGHK07BPXD-qu-ufSL6dfVkSEzyk9p+_hL7AhmkZhu4eF11kKA@mail.gmail.com>
Subject: Re: [PATCH net-next] net: bpf: fix request_sock leak in filter.c
To:     Antoine Tenart <atenart@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        David Miller <davem@davemloft.net>, cjebpub@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jun 7, 2022 at 6:18 PM Antoine Tenart <atenart@kernel.org> wrote:
>
> Hi Jon,
>
> This patch is targeted at the networking subsystem, as such (see the
> "NETWORKING [GENERAL]" section in MAINTAINERS), you should send it to
> netdev@vger.kernel.org and to the networking maintainers (David, Jakub,
> Paolo & Eric).
>
> This also fixes an issue and should be targeted at [net] instead of
> [net-next]. Because of this you'll also need a Fixes: tag.
>
> Quoting Jon Maxwell (2022-06-07 03:38:44)
> > A customer reported a request_socket leak in a Calico cloud environment. We
> > found that a BPF program was doing a socket lookup with takes a refcnt on
> > the socket and that it was finding the request_socket but returning the parent
> > LISTEN socket via sk_to_full_sk() without decrementing the child request socket
> > 1st, resulting in request_sock slab object leak. This patch retains the
> > existing behaviour of returning full socks to the caller but it also decrements
> > the child request_socket if one is present before doing so to prevent the leak.
> >
> > Thanks to Curtis Taylor for all the help in diagnosing and testing this. And
> > thanks to Antoine Tenart for the reproducer and patch input.
> >
> > Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
> > Tested-by: Curtis Taylor <cjebpub@gmail.com>
> > Co-developed-by: Antoine Tenart <atenart@kernel.org>
>
> You need to put my SoB here when using the above tag. You'll also need
> to put your SoB at the end of all the above tags instead of the top.
>
> > @@ -6514,13 +6514,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >  {
> >         struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
> >                                            ifindex, proto, netns_id, flags);
> > +       struct sock *sk1 = sk;
> >
> >         if (sk) {
> >                 sk = sk_to_full_sk(sk);
> > -               if (!sk_fullsock(sk)) {
> > -                       sock_gen_put(sk);
>
> I'd suggest to add a comment here to explain why sock_gen_put is called
> on the original sk.
>
> > +               if (!sk_fullsock(sk1))
> > +                       sock_gen_put(sk1);
> > +               if (!sk_fullsock(sk))
> >                         return NULL;
> > -               }
> >         }
> >
> >         return sk;
> > @@ -6551,13 +6552,14 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >  {
> >         struct sock *sk = bpf_skc_lookup(skb, tuple, len, proto, netns_id,
> >                                          flags);
> > +       struct sock *sk1 = sk;
> >
> >         if (sk) {
> >                 sk = sk_to_full_sk(sk);
> > -               if (!sk_fullsock(sk)) {
> > -                       sock_gen_put(sk);
>
> Ditto.
>
> > +               if (!sk_fullsock(sk1))
> > +                       sock_gen_put(sk1);
> > +               if (!sk_fullsock(sk))
> >                         return NULL;
> > -               }
> >         }
> >
> >         return sk;
>
> Thanks!
> Antoine

Thanks Antoine, ack I'll do that and resubmit.