[PATCH bpf-next 0/2] bpf: bpf trampoline improvements

[PATCH bpf-next 0/2] bpf: bpf trampoline improvements

[Yafang Shao]

Patch #1: Fix memleak in bpf trampoline found on our server
Patch #2: Improve the trampoline ksym name 

Yafang Shao (2):
  bpf: Fix memleak due to fentry attach failure
  bpf: Show total linked progs cnt instead of selector in trampoline
    ksym

 include/linux/bpf.h     |  1 -
 kernel/bpf/trampoline.c | 24 +++++++++++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

-- 
1.8.3.1

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

[Yafang Shao]

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(-)

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)
-- 
1.8.3.1

[PATCH bpf-next 2/2] bpf: Show total linked progs cnt instead of selector in trampoline ksym

[Yafang Shao]

After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
is only used to indicate how many times the bpf trampoline image are
updated and been displayed in the trampoline ksym name. After the
trampoline is freed, the count will start from 0 again.
So the count is a useless value to the user, we'd better
show a more meaningful value like how many progs are linked to this
trampoline. After that change, the selector can be removed eventally.
If the user want to check whether the bpf trampoline image has been updated
or not, the user can also compare the address. Each time the trampoline
image is updated, the address will change consequently.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/bpf.h     | 1 -
 kernel/bpf/trampoline.c | 7 ++-----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 456f33b..36e4b2d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1125,7 +1125,6 @@ struct bpf_trampoline {
 	int progs_cnt[BPF_TRAMP_MAX];
 	/* Executable image of trampoline */
 	struct bpf_tramp_image *cur_image;
-	u64 selector;
 	struct module *mod;
 };
 
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 7067cdf..be37d87 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -410,11 +410,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 		err = unregister_fentry(tr, tr->cur_image->image);
 		bpf_tramp_image_put(tr->cur_image);
 		tr->cur_image = NULL;
-		tr->selector = 0;
 		goto out;
 	}
 
-	im = bpf_tramp_image_alloc(tr->key, tr->selector);
+	im = bpf_tramp_image_alloc(tr->key, total);
 	if (IS_ERR(im)) {
 		err = PTR_ERR(im);
 		goto out;
@@ -451,8 +450,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 
 	set_memory_rox((long)im->image, 1);
 
-	WARN_ON(tr->cur_image && tr->selector == 0);
-	WARN_ON(!tr->cur_image && tr->selector);
+	WARN_ON(tr->cur_image && total == 0);
 	if (tr->cur_image)
 		/* progs already running at this address */
 		err = modify_fentry(tr, tr->cur_image->image, im->image, lock_direct_mutex);
@@ -482,7 +480,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 	if (tr->cur_image)
 		bpf_tramp_image_put(tr->cur_image);
 	tr->cur_image = im;
-	tr->selector++;
 out:
 	/* If any error happens, restore previous flags */
 	if (err)
-- 
1.8.3.1

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

[Song Liu]

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
[...]

Re: [PATCH bpf-next 2/2] bpf: Show total linked progs cnt instead of selector in trampoline ksym

[Song Liu]

On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
> is only used to indicate how many times the bpf trampoline image are
> updated and been displayed in the trampoline ksym name. After the
> trampoline is freed, the count will start from 0 again.
> So the count is a useless value to the user, we'd better
> show a more meaningful value like how many progs are linked to this
> trampoline. After that change, the selector can be removed eventally.
> If the user want to check whether the bpf trampoline image has been updated
> or not, the user can also compare the address. Each time the trampoline
> image is updated, the address will change consequently.

I wonder whether this will cause confusion to some users. Maybe the saving
doesn't worth the churn.

Song

>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/bpf.h     | 1 -
>  kernel/bpf/trampoline.c | 7 ++-----
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 456f33b..36e4b2d 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1125,7 +1125,6 @@ struct bpf_trampoline {
>         int progs_cnt[BPF_TRAMP_MAX];
>         /* Executable image of trampoline */
>         struct bpf_tramp_image *cur_image;
> -       u64 selector;
>         struct module *mod;
>  };
>
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 7067cdf..be37d87 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -410,11 +410,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>                 err = unregister_fentry(tr, tr->cur_image->image);
>                 bpf_tramp_image_put(tr->cur_image);
>                 tr->cur_image = NULL;
> -               tr->selector = 0;
>                 goto out;
>         }
>
> -       im = bpf_tramp_image_alloc(tr->key, tr->selector);
> +       im = bpf_tramp_image_alloc(tr->key, total);
>         if (IS_ERR(im)) {
>                 err = PTR_ERR(im);
>                 goto out;
> @@ -451,8 +450,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>
>         set_memory_rox((long)im->image, 1);
>
> -       WARN_ON(tr->cur_image && tr->selector == 0);
> -       WARN_ON(!tr->cur_image && tr->selector);
> +       WARN_ON(tr->cur_image && total == 0);
>         if (tr->cur_image)
>                 /* progs already running at this address */
>                 err = modify_fentry(tr, tr->cur_image->image, im->image, lock_direct_mutex);
> @@ -482,7 +480,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>         if (tr->cur_image)
>                 bpf_tramp_image_put(tr->cur_image);
>         tr->cur_image = im;
> -       tr->selector++;
>  out:
>         /* If any error happens, restore previous flags */
>         if (err)
> --
> 1.8.3.1
>
>

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

[Yafang Shao]

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?

-- 
Regards
Yafang

Re: [PATCH bpf-next 2/2] bpf: Show total linked progs cnt instead of selector in trampoline ksym

[Yafang Shao]

On Wed, May 10, 2023 at 1:43 AM Song Liu <song@kernel.org> wrote:
>
> On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
> > is only used to indicate how many times the bpf trampoline image are
> > updated and been displayed in the trampoline ksym name. After the
> > trampoline is freed, the count will start from 0 again.
> > So the count is a useless value to the user, we'd better
> > show a more meaningful value like how many progs are linked to this
> > trampoline. After that change, the selector can be removed eventally.
> > If the user want to check whether the bpf trampoline image has been updated
> > or not, the user can also compare the address. Each time the trampoline
> > image is updated, the address will change consequently.
>
> I wonder whether this will cause confusion to some users. Maybe the saving
> doesn't worth the churn.

The trampoline ksym name as such:
ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]

I don't know what the user may use the selector for. It seems that the
selector is meaningless. While the cnt of linked progs can really help
users, with which the user can easily figure out how many progs are
linked to a kernel function.

However the key in the ksym name really confused me before, because
its meaning was changed.

In the old kernel(for example, the 4.19), it is
  bpf_trampoline_[BTF_ID]
while in the new kernel, it is
  bpf_trampoline_[ OBJ_ID | BTF_ID ]_[ SELECTOR ]

But I think that is not a big problem, because the user tools can be
changed to (key & 0x7fffffff) to make it backward compatible.

Currently there's no document on the name, so we can choose what we
prefer. After we doc it, we have to keep it backward compatible.
BTW, I think we should add a document on the name, otherwise the user
has to read the kernel code to parse it.

-- 
Regards
Yafang

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

[Song Liu]

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

Re: [PATCH bpf-next 2/2] bpf: Show total linked progs cnt instead of selector in trampoline ksym

[Song Liu]

On Tue, May 9, 2023 at 7:56 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Wed, May 10, 2023 at 1:43 AM Song Liu <song@kernel.org> wrote:
> >
> > On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
> > > is only used to indicate how many times the bpf trampoline image are
> > > updated and been displayed in the trampoline ksym name. After the
> > > trampoline is freed, the count will start from 0 again.
> > > So the count is a useless value to the user, we'd better
> > > show a more meaningful value like how many progs are linked to this
> > > trampoline. After that change, the selector can be removed eventally.
> > > If the user want to check whether the bpf trampoline image has been updated
> > > or not, the user can also compare the address. Each time the trampoline
> > > image is updated, the address will change consequently.
> >
> > I wonder whether this will cause confusion to some users. Maybe the saving
> > doesn't worth the churn.
>
> The trampoline ksym name as such:
> ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]
>
> I don't know what the user may use the selector for. It seems that the
> selector is meaningless. While the cnt of linked progs can really help
> users, with which the user can easily figure out how many progs are
> linked to a kernel function.

Hmm, agreed that the chance to break user space is low. Maybe we can just
remove it? IOW, only keep bpf_trampoline_6442453466

Thanks,
Song

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

[Yafang Shao]

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.

-- 
Regards
Yafang

Re: [PATCH bpf-next 2/2] bpf: Show total linked progs cnt instead of selector in trampoline ksym

[Yafang Shao]

On Wed, May 10, 2023 at 2:30 PM Song Liu <song@kernel.org> wrote:
>
> On Tue, May 9, 2023 at 7:56 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Wed, May 10, 2023 at 1:43 AM Song Liu <song@kernel.org> wrote:
> > >
> > > On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > >
> > > > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
> > > > is only used to indicate how many times the bpf trampoline image are
> > > > updated and been displayed in the trampoline ksym name. After the
> > > > trampoline is freed, the count will start from 0 again.
> > > > So the count is a useless value to the user, we'd better
> > > > show a more meaningful value like how many progs are linked to this
> > > > trampoline. After that change, the selector can be removed eventally.
> > > > If the user want to check whether the bpf trampoline image has been updated
> > > > or not, the user can also compare the address. Each time the trampoline
> > > > image is updated, the address will change consequently.
> > >
> > > I wonder whether this will cause confusion to some users. Maybe the saving
> > > doesn't worth the churn.
> >
> > The trampoline ksym name as such:
> > ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]
> >
> > I don't know what the user may use the selector for. It seems that the
> > selector is meaningless. While the cnt of linked progs can really help
> > users, with which the user can easily figure out how many progs are
> > linked to a kernel function.
>
> Hmm, agreed that the chance to break user space is low. Maybe we can just
> remove it? IOW, only keep bpf_trampoline_6442453466
>

Agree. I will do it as you suggested.

-- 
Regards
Yafang

Re: [PATCH bpf-next 2/2] bpf: Show total linked progs cnt instead of selector in trampoline ksym

[Jiri Olsa]

On Wed, May 10, 2023 at 11:33:21PM +0800, Yafang Shao wrote:
> On Wed, May 10, 2023 at 2:30 PM Song Liu <song@kernel.org> wrote:
> >
> > On Tue, May 9, 2023 at 7:56 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > On Wed, May 10, 2023 at 1:43 AM Song Liu <song@kernel.org> wrote:
> > > >
> > > > On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > > >
> > > > > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
> > > > > is only used to indicate how many times the bpf trampoline image are
> > > > > updated and been displayed in the trampoline ksym name. After the
> > > > > trampoline is freed, the count will start from 0 again.
> > > > > So the count is a useless value to the user, we'd better
> > > > > show a more meaningful value like how many progs are linked to this
> > > > > trampoline. After that change, the selector can be removed eventally.
> > > > > If the user want to check whether the bpf trampoline image has been updated
> > > > > or not, the user can also compare the address. Each time the trampoline
> > > > > image is updated, the address will change consequently.
> > > >
> > > > I wonder whether this will cause confusion to some users. Maybe the saving
> > > > doesn't worth the churn.
> > >
> > > The trampoline ksym name as such:
> > > ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]
> > >
> > > I don't know what the user may use the selector for. It seems that the
> > > selector is meaningless. While the cnt of linked progs can really help
> > > users, with which the user can easily figure out how many progs are
> > > linked to a kernel function.
> >
> > Hmm, agreed that the chance to break user space is low. Maybe we can just
> > remove it? IOW, only keep bpf_trampoline_6442453466
> >
> 
> Agree. I will do it as you suggested.

perf is actually is still trying parse the old name

        /* .. and only for trampolines and dispatchers */
        if ((sscanf(name, "bpf_trampoline_%lu", &id) == 1) ||
            (sscanf(name, "bpf_dispatcher_%s", disp) == 1))
                err = process_bpf_image(name, start, data);

so this change would actualy fix it ;-)

thanks,
jirka