Messages in this thread | | | From | Jonathan Maxwell <> | Date | Wed, 8 Jun 2022 08:38:34 +1000 | Subject | Re: [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.
| |