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,
> > >
