| Message ID | 20230509151511.3937-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: bpf trampoline improvements | expand |
On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> 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. > > 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@gmail.com> I guess we need: Fixes: e21aa341785c ("bpf: Fix fexit trampoline.") > --- > 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? Thanks, Song [...]
On Wed, May 10, 2023 at 1:41 AM Song Liu <song@kernel.org> wrote: > > On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> 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@gmail.com> > > 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?
On Tue, May 9, 2023 at 7:39 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > On Wed, May 10, 2023 at 1:41 AM Song Liu <song@kernel.org> wrote: [...] > > > +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? How about we also use kfree_rcu() in bpf_tramp_image_free()? Thanks, Song
On Wed, May 10, 2023 at 2:24 PM Song Liu <song@kernel.org> wrote: > > On Tue, May 9, 2023 at 7:39 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > On Wed, May 10, 2023 at 1:41 AM Song Liu <song@kernel.org> wrote: > [...] > > > > +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? > > How about we also use kfree_rcu() in bpf_tramp_image_free()? > Looks good. I will change it.
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); +} + static void __bpf_tramp_image_put_deferred(struct work_struct *work) { struct bpf_tramp_image *im; @@ -438,7 +447,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut &tr->func.model, tr->flags, tlinks, tr->func.addr); if (err < 0) - goto out; + goto out_free; set_memory_rox((long)im->image, 1); @@ -468,7 +477,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut } #endif if (err) - goto out; + goto out_free; if (tr->cur_image) bpf_tramp_image_put(tr->cur_image); @@ -480,6 +489,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut tr->flags = orig_flags; kfree(tlinks); return err; + +out_free: + bpf_tramp_image_free(im); + goto out; } static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
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; } 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@gmail.com> --- kernel/bpf/trampoline.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)