Re: [PATCH net-next] net: bpf: fix request_sock leak in filter.c

[Jonathan Maxwell]

On Tue, Jun 7, 2022 at 6:18 PM Antoine Tenart <atenart@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx 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@xxxxxxxxx>
> > Tested-by: Curtis Taylor <cjebpub@xxxxxxxxx>
> > Co-developed-by: Antoine Tenart <atenart@xxxxxxxxxx>
>
> 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.