Re: [PATCH bpf-next 1/2] bpf: Fix memleak due to fentry attach failure

[Yafang Shao <laoar.shao@xxxxxxxxx>]

On Wed, May 10, 2023 at 1:41 AM Song Liu <song@xxxxxxxxxx> wrote:
>
> On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> >
> > If it fails to attach fentry, the allocated bpf trampoline image will be
> > left in the system. That can be verified by checking /proc/kallsyms.
> >
> > This meamleak can be verified by a simple bpf program as follows,
> >
> >   SEC("fentry/trap_init")
> >   int fentry_run()
> >   {
> >       return 0;
> >   }
>
> Nice trick! We can build some interesting tests with trap_init.
>

Good suggestion. I will think about it.

> >
> > It will fail to attach trap_init because this function is freed after
> > kernel init, and then we can find the trampoline image is left in the
> > system by checking /proc/kallsyms.
> >   $ tail /proc/kallsyms
> >   ffffffffc0613000 t bpf_trampoline_6442453466_1  [bpf]
> >   ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]
> >
> >   $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'"
> >   [2522] FUNC 'trap_init' type_id=119 linkage=static
> >
> >   $ echo $((6442453466 & 0x7fffffff))
> >   2522
> >
> > Note that there are two left bpf trampoline images, that is because the
> > libbpf will fallback to raw tracepoint if -EINVAL is returned.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
>
> I guess we need:
>
>  Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
>

Thanks for pointing it out. I will add it.

> > ---
> >  kernel/bpf/trampoline.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index ac021bc..7067cdf 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -251,6 +251,15 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
> >         return tlinks;
> >  }
> >
> > +static void bpf_tramp_image_free(struct bpf_tramp_image *im)
> > +{
> > +       bpf_image_ksym_del(&im->ksym);
> > +       bpf_jit_free_exec(im->image);
> > +       bpf_jit_uncharge_modmem(PAGE_SIZE);
> > +       percpu_ref_exit(&im->pcref);
> > +       kfree(im);
> > +}
>
> Can we share some of this function with __bpf_tramp_image_put_deferred?
>

It seems we can introduce a generic helper as follows,
  static void __bpf_tramp_image_free(struct bpf_tramp_image *im)
  {
      bpf_image_ksym_del(&im->ksym);
      bpf_jit_free_exec(im->image);
      bpf_jit_uncharge_modmem(PAGE_SIZE);
      percpu_ref_exit(&im->pcref);
  }

And then use it in both bpf_tramp_image_free() and
__bpf_tramp_image_put_deferred().
WDYT?

-- 
Regards
Yafang