| Message ID | 20230113093427.1666466-1-imagedong@tencent.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2fa074536590a82ee6c670d5f119f3dfb4ca6015 |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: replace '.' with '_' in legacy kprobe event name | expand |
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, >
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, > >
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!
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, > > >
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, >>>>
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, > >>>>
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,