Re: [PATCH] libbpf: replace '.' with '_' in legacy kprobe event name

[Menglong Dong]

Hello,

On Sat, Jan 14, 2023 at 6:07 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Jan 13, 2023 at 6:13 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> >
> > On 13/01/2023 09:34, menglong8.dong@xxxxxxxxx wrote:
> > > From: Menglong Dong <imagedong@xxxxxxxxxxx>
> > >
> > > '.' is not allowed in the event name of kprobe. Therefore, we will get a
> > > EINVAL if the kernel function name has a '.' in legacy kprobe attach
> > > case, such as 'icmp_reply.constprop.0'.
> > >
> > > In order to adapt this case, we need to replace the '.' with other char
> > > in gen_kprobe_legacy_event_name(). And I use '_' for this propose.
> > >
> > > Signed-off-by: Menglong Dong <imagedong@xxxxxxxxxxx>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index fdfb1ca34ced..5d6f6675c2f2 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -9994,9 +9994,16 @@ static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
> > >                                        const char *kfunc_name, size_t offset)
> > >  {
> > >       static int index = 0;
> > > +     int i = 0;
> > >
> > >       snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx_%d", getpid(), kfunc_name, offset,
> > >                __sync_fetch_and_add(&index, 1));
> > > +
> > > +     while (buf[i] != '\0') {
> > > +             if (buf[i] == '.')
> > > +                     buf[i] = '_';
> > > +             i++;
> > > +     }
> > >  }
> >
> > probably more naturally expressed as a for() loop as is done in
> > gen_uprobe_legacy_event_name(), but not a big deal.
> >
> > Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>
> Applied, but tuned to be exactly the same loop as in
> gen_uprobe_legacy_event_name. Thanks.
>

Thanks for your modification, it looks much better now!

> >
> > One issue with the legacy kprobe code is that we don't get test coverage
> > with it on new kernels - I wonder if it would be worth adding a force_legacy
> > option to bpf_kprobe_opts? A separate issue to this change of course, but
> > if we had that we could add some legacy kprobe tests that would run
> > for new kernels as well.
>
> Yep, good idea. If we ever have some bug in the latest greatest kprobe
> implementation, users will have an option to work around that with
> this.
>
> The only thing is that we already have 3 modes: legacy, perf-based
> through ioctl, and bpf_link-based, so I think it should be something
> like
>
> enum kprobe_mode {
>     KPROBE_MODE_DEFAULT = 0, /* latest supported by kernel */
>     KPROBE_MODE_LEGACY,
>     KPROBE_MODE_PERF,
>     KPROBE_MODE_LINK,
> };
>
> LEGACY/PERF/LINK naming should be thought through, just a quick example.
>
> And then just have `enum kprobe_mode mode;` in kprobe_opts, which
> would default to 0 (KPROBE_MODE_DEFAULT).
>
> Would that work?
>

Sounds great, which means I don't have to switch to an older
kernel to test this function for my app.

BTW, should I do this job, (which is my pleasure), or Alan?


Thanks!
Menglong Dong

> >
> > Alan
> > >
> > >  static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,
> > >