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

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

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

'.' 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@tencent.com>
---
 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++;
+	}
 }
 
 static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,
-- 
2.39.0

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

[Alan Maguire]

On 13/01/2023 09:34, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> '.' 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@tencent.com>
> ---
>  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@oracle.com>

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.

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

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

[Andrii Nakryiko]

On Fri, Jan 13, 2023 at 6:13 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 13/01/2023 09:34, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > '.' 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@tencent.com>
> > ---
> >  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@oracle.com>

Applied, but tuned to be exactly the same loop as in
gen_uprobe_legacy_event_name. Thanks.

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

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Fri, 13 Jan 2023 17:34:27 +0800 you wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> '.' 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.
> 
> [...]

Here is the summary with links:
  - libbpf: replace '.' with '_' in legacy kprobe event name
    https://git.kernel.org/bpf/bpf-next/c/2fa074536590

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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@gmail.com> wrote:
>
> On Fri, Jan 13, 2023 at 6:13 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On 13/01/2023 09:34, menglong8.dong@gmail.com wrote:
> > > From: Menglong Dong <imagedong@tencent.com>
> > >
> > > '.' 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@tencent.com>
> > > ---
> > >  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@oracle.com>
>
> 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,
> > >

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@gmail.com> wrote:
>>
>> On Fri, Jan 13, 2023 at 6:13 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>
>>> On 13/01/2023 09:34, menglong8.dong@gmail.com wrote:
>>>> From: Menglong Dong <imagedong@tencent.com>
>>>>
>>>> '.' 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@tencent.com>
>>>> ---
>>>>  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@oracle.com>
>>
>> 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,
>>>>

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@oracle.com> wrote:
>
> On 16/01/2023 02:27, Menglong Dong wrote:
> > Hello,
> >
> > On Sat, Jan 14, 2023 at 6:07 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Fri, Jan 13, 2023 at 6:13 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >>>
> >>> On 13/01/2023 09:34, menglong8.dong@gmail.com wrote:
> >>>> From: Menglong Dong <imagedong@tencent.com>
> >>>>
> >>>> '.' 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@tencent.com>
> >>>> ---
> >>>>  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@oracle.com>
> >>
> >> 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,
> >>>>