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

[Menglong Dong]

On Mon, Jan 16, 2023 at 6:27 PM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On 16/01/2023 02:27, Menglong Dong wrote:
> > 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?
> >>
>
> Looks good - I'd missed the "no BPF link" case, it'd be great to test that too.
>

My mistake, I forgot to add the 'bpf-next' tag :)

> So for legacy mode, we'd force using the legacy codepath, and to simulate the
> PERF mode where BPF link isn't supported I think we'd need to add to bpf_perf_event_opts
> so that we could choose the "no bpf link" code path rather than purely relying on the
> kernel support test.
>
> This would be nice as it would allow us to test other "pre-BPF link" attach targets
> too.
>
> So I think we need add an option to bpf_perf_event_opts for when KPROBE_MODE_PERF is set,
> such as PE_MODE_PERF or PE_MODE_NO_BPF_LINK.
>
> All of this would generalize to uprobe too I think; having a perf option makes that
> straightforward I suspect.
>
> >
> > 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?
> >
> >
>
> Feel free to take this on if you've got time; I'm trying to get the dwarves patches
> covering support for .isra functions out as soon as possible so it would probably be
> a week or so before I get to this. Something like the above combined with updating
> the attach_probe selftests to run through the various attach modes would be great for
> testing legacy codepaths on newer kernels. We could perhaps rework the test_attach_probe()
> function to take an attach mode argument, and add subtests for each attach mode (skipping
> if it wasn't supported). Thanks!
>

Okay, I'll have a try!

> Alan
>
> > Thanks!
> > Menglong Dong
> >
> >>>
> >>> Alan
> >>>>
> >>>>  static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,
> >>>>