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

[Alan Maguire]

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.

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!

Alan

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