lkml.org 
[lkml]   [2022]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH net-next] net: bpf: fix request_sock leak in filter.c
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.

\
 
 \ /
  Last update: 2022-06-08 04:55    [W:0.043 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site