On Fri, Jun 10, 2022 at 10:36 AM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Thu, Jun 09, 2022 at 05:17:47PM -0700, Martin KaFai Lau wrote:
> > On Thu, Jun 09, 2022 at 10:29:15PM +0200, Daniel Borkmann wrote:
> > > On 6/9/22 3:18 AM, Jon Maxwell wrote:
> > > > 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
> > Great catch and debug indeed!
> >
> > > > 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.
> > > >
> > > > Fixes: f7355a6c0497 bpf: ("Check sk_fullsock() before returning from bpf_sk_lookup()")
> > > > Fixes: edbf8c01de5a bpf: ("add skc_lookup_tcp helper")
> > Instead of the above commits, I think this dated back to
> > 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
>
> Since this is more bpf specific, I think it could go to the bpf tree.
> In v2, please cc bpf@xxxxxxxxxxxxxxx and tag it with 'PATCH v2 bpf'.
Okay thanks will do.
Daniel, are you okay with omitting 'if (unlikely...) { WARN_ONCE(...); }'?
If so I'll stick to the rest of the logic of your suggestion and omit that
check in v1.
Regards
Jon
