| Message ID | 20220326144320.560939-1-hengqi.chen@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: Allow kprobe attach using legacy debugfs interface | expand |
On Sat, Mar 26, 2022 at 7:43 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > On some old kernels, kprobe auto-attach may fail when attach to symbols > like udp_send_skb.isra.52 . This is because the kernel has kprobe PMU > but don't allow attach to a symbol with '.' ([0]). Add a new option to > bpf_kprobe_opts to allow using the legacy kprobe attach directly. > This way, users can use bpf_program__attach_kprobe_opts in a dedicated > custom sec handler to handle such case. > > [0]: https://github.com/torvalds/linux/blob/v4.18/kernel/trace/trace_kprobe.c#L340-L343 > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- It's sad, but it makes sense. But, let's have a selftests that validates uses legacy option explicitly (e.g., in prog_tests/attach_probe.c). Also, let's fix this limitation in the kernel? It makes no sense to limit attaching to a proper kallsym symbol. > tools/lib/bpf/libbpf.c | 9 ++++++++- > tools/lib/bpf/libbpf.h | 4 +++- > 2 files changed, 11 insertions(+), 2 deletions(-) > [...]
Hello, Andrii On 2022/3/30 7:18 AM, Andrii Nakryiko wrote: > On Sat, Mar 26, 2022 at 7:43 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: >> >> On some old kernels, kprobe auto-attach may fail when attach to symbols >> like udp_send_skb.isra.52 . This is because the kernel has kprobe PMU >> but don't allow attach to a symbol with '.' ([0]). Add a new option to >> bpf_kprobe_opts to allow using the legacy kprobe attach directly. >> This way, users can use bpf_program__attach_kprobe_opts in a dedicated >> custom sec handler to handle such case. >> >> [0]: https://github.com/torvalds/linux/blob/v4.18/kernel/trace/trace_kprobe.c#L340-L343 >> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >> --- > > It's sad, but it makes sense. But, let's have a selftests that > validates uses legacy option explicitly (e.g., in > prog_tests/attach_probe.c). Also, let's fix this limitation in the OK, will add a selftest to exercise the new option. > kernel? It makes no sense to limit attaching to a proper kallsym > symbol. This limitation is lifted in newer kernel. Kernel v5.4 don't have this issue. > >> tools/lib/bpf/libbpf.c | 9 ++++++++- >> tools/lib/bpf/libbpf.h | 4 +++- >> 2 files changed, 11 insertions(+), 2 deletions(-) >> > > [...] -- Hengqi
On Tue, Mar 29, 2022 at 7:30 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > Hello, Andrii > > On 2022/3/30 7:18 AM, Andrii Nakryiko wrote: > > On Sat, Mar 26, 2022 at 7:43 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > >> > >> On some old kernels, kprobe auto-attach may fail when attach to symbols > >> like udp_send_skb.isra.52 . This is because the kernel has kprobe PMU > >> but don't allow attach to a symbol with '.' ([0]). Add a new option to > >> bpf_kprobe_opts to allow using the legacy kprobe attach directly. > >> This way, users can use bpf_program__attach_kprobe_opts in a dedicated > >> custom sec handler to handle such case. > >> > >> [0]: https://github.com/torvalds/linux/blob/v4.18/kernel/trace/trace_kprobe.c#L340-L343 > >> > >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > >> --- > > > > It's sad, but it makes sense. But, let's have a selftests that > > validates uses legacy option explicitly (e.g., in > > prog_tests/attach_probe.c). Also, let's fix this limitation in the > > OK, will add a selftest to exercise the new option. > > > kernel? It makes no sense to limit attaching to a proper kallsym > > symbol. > > This limitation is lifted in newer kernel. Kernel v5.4 don't have this issue. Oh, ok. So how about another plan of attack then: if kprobe target function has '.' *and* we are on the kernel that doesn't support that, switch to legacy kprobe automatically? No need for a new option, libbpf handles this transparently. Still need a test for kprobe with '.' in it, though not sure how reliable that will be... We can use kallsyms cache to check if expected xxx.isra.0 (or whatever) is present, and if not - skip subtest? > > > > >> tools/lib/bpf/libbpf.c | 9 ++++++++- > >> tools/lib/bpf/libbpf.h | 4 +++- > >> 2 files changed, 11 insertions(+), 2 deletions(-) > >> > > > > [...] > > -- > Hengqi
On 2022/3/30 10:50 AM, Andrii Nakryiko wrote: > On Tue, Mar 29, 2022 at 7:30 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: >> >> Hello, Andrii >> >> On 2022/3/30 7:18 AM, Andrii Nakryiko wrote: >>> On Sat, Mar 26, 2022 at 7:43 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: >>>> >>>> On some old kernels, kprobe auto-attach may fail when attach to symbols >>>> like udp_send_skb.isra.52 . This is because the kernel has kprobe PMU >>>> but don't allow attach to a symbol with '.' ([0]). Add a new option to >>>> bpf_kprobe_opts to allow using the legacy kprobe attach directly. >>>> This way, users can use bpf_program__attach_kprobe_opts in a dedicated >>>> custom sec handler to handle such case. >>>> >>>> [0]: https://github.com/torvalds/linux/blob/v4.18/kernel/trace/trace_kprobe.c#L340-L343 >>>> >>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >>>> --- >>> >>> It's sad, but it makes sense. But, let's have a selftests that >>> validates uses legacy option explicitly (e.g., in >>> prog_tests/attach_probe.c). Also, let's fix this limitation in the >> >> OK, will add a selftest to exercise the new option. >> >>> kernel? It makes no sense to limit attaching to a proper kallsym >>> symbol. >> >> This limitation is lifted in newer kernel. Kernel v5.4 don't have this issue. > > Oh, ok. So how about another plan of attack then: if kprobe target > function has '.' *and* we are on the kernel that doesn't support that, > switch to legacy kprobe automatically? No need for a new option, > libbpf handles this transparently. > That's better, and also eliminate the need for custom SEC() handler. > Still need a test for kprobe with '.' in it, though not sure how > reliable that will be... We can use kallsyms cache to check if > expected xxx.isra.0 (or whatever) is present, and if not - skip > subtest? > Not sure how to do that. Even if such symbol exists, how to reliably trigger it is another problem. >> >>> >>>> tools/lib/bpf/libbpf.c | 9 ++++++++- >>>> tools/lib/bpf/libbpf.h | 4 +++- >>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>> >>> >>> [...] >> >> -- >> Hengqi
On Wed, 30 Mar 2022, Hengqi Chen wrote: > > > On 2022/3/30 10:50 AM, Andrii Nakryiko wrote: > > On Tue, Mar 29, 2022 at 7:30 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > >> > >> Hello, Andrii > >> > >> On 2022/3/30 7:18 AM, Andrii Nakryiko wrote: > >>> On Sat, Mar 26, 2022 at 7:43 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > >>>> > >>>> On some old kernels, kprobe auto-attach may fail when attach to symbols > >>>> like udp_send_skb.isra.52 . This is because the kernel has kprobe PMU > >>>> but don't allow attach to a symbol with '.' ([0]). Add a new option to > >>>> bpf_kprobe_opts to allow using the legacy kprobe attach directly. > >>>> This way, users can use bpf_program__attach_kprobe_opts in a dedicated > >>>> custom sec handler to handle such case. > >>>> > >>>> [0]: https://github.com/torvalds/linux/blob/v4.18/kernel/trace/trace_kprobe.c#L340-L343 > >>>> > >>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > >>>> --- > >>> > >>> It's sad, but it makes sense. But, let's have a selftests that > >>> validates uses legacy option explicitly (e.g., in > >>> prog_tests/attach_probe.c). Also, let's fix this limitation in the > >> > >> OK, will add a selftest to exercise the new option. > >> > >>> kernel? It makes no sense to limit attaching to a proper kallsym > >>> symbol. > >> > >> This limitation is lifted in newer kernel. Kernel v5.4 don't have this issue. > > > > Oh, ok. So how about another plan of attack then: if kprobe target > > function has '.' *and* we are on the kernel that doesn't support that, > > switch to legacy kprobe automatically? No need for a new option, > > libbpf handles this transparently. > > > > That's better, and also eliminate the need for custom SEC() handler. > > > Still need a test for kprobe with '.' in it, though not sure how > > reliable that will be... We can use kallsyms cache to check if > > expected xxx.isra.0 (or whatever) is present, and if not - skip > > subtest? > > > > Not sure how to do that. Even if such symbol exists, how to reliably > trigger it is another problem. > could we add a function to bpf testmod that is easily triggered and likely to be .isra-ed maybe? Experimenting, the following function becomes .isra-ed at when compiled with -fipa-sra: diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf index e585e1c..bb51e21 100644 --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c @@ -88,6 +88,17 @@ __weak noinline struct file *bpf_testmod_return_ptr(int arg) } } +struct testisra { + int val1; + int val2; + int val3; +}; + +static noinline void bpf_testmod_test_isra(struct testisra *t, int val1, int val2) +{ + t->val3 = val1 + val2; +} + noinline ssize_t bpf_testmod_test_read(struct file *file, struct kobject *kobj, struct bin_attribute *bin_attr, @@ -98,8 +109,14 @@ __weak noinline struct file *bpf_testmod_return_ptr(int arg) .off = off, .len = len, }; + struct testisra t = { + .val1 = off, + .val2 = len + }; int i = 1; + bpf_testmod_test_isra(&t, t.val1, t.val2); + while (bpf_testmod_return_ptr(i)) i++; Tested on gcc 9; possibly different results on different versions.. Alan
Hello, Alan On 2022/3/31 5:27 PM, Alan Maguire wrote: > On Wed, 30 Mar 2022, Hengqi Chen wrote: > >> >> >> On 2022/3/30 10:50 AM, Andrii Nakryiko wrote: >>> On Tue, Mar 29, 2022 at 7:30 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: >>>> >>>> Hello, Andrii >>>> >>>> On 2022/3/30 7:18 AM, Andrii Nakryiko wrote: >>>>> On Sat, Mar 26, 2022 at 7:43 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: >>>>>> >>>>>> On some old kernels, kprobe auto-attach may fail when attach to symbols >>>>>> like udp_send_skb.isra.52 . This is because the kernel has kprobe PMU >>>>>> but don't allow attach to a symbol with '.' ([0]). Add a new option to >>>>>> bpf_kprobe_opts to allow using the legacy kprobe attach directly. >>>>>> This way, users can use bpf_program__attach_kprobe_opts in a dedicated >>>>>> custom sec handler to handle such case. >>>>>> >>>>>> [0]: https://github.com/torvalds/linux/blob/v4.18/kernel/trace/trace_kprobe.c#L340-L343 >>>>>> >>>>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >>>>>> --- >>>>> >>>>> It's sad, but it makes sense. But, let's have a selftests that >>>>> validates uses legacy option explicitly (e.g., in >>>>> prog_tests/attach_probe.c). Also, let's fix this limitation in the >>>> >>>> OK, will add a selftest to exercise the new option. >>>> >>>>> kernel? It makes no sense to limit attaching to a proper kallsym >>>>> symbol. >>>> >>>> This limitation is lifted in newer kernel. Kernel v5.4 don't have this issue. >>> >>> Oh, ok. So how about another plan of attack then: if kprobe target >>> function has '.' *and* we are on the kernel that doesn't support that, >>> switch to legacy kprobe automatically? No need for a new option, >>> libbpf handles this transparently. >>> >> >> That's better, and also eliminate the need for custom SEC() handler. >> >>> Still need a test for kprobe with '.' in it, though not sure how >>> reliable that will be... We can use kallsyms cache to check if >>> expected xxx.isra.0 (or whatever) is present, and if not - skip >>> subtest? >>> >> >> Not sure how to do that. Even if such symbol exists, how to reliably >> trigger it is another problem. >> > > could we add a function to bpf testmod that is easily triggered > and likely to be .isra-ed maybe? > > Experimenting, the following function becomes .isra-ed at when > compiled with -fipa-sra: > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > b/tools/testing/selftests/bpf/bpf > index e585e1c..bb51e21 100644 > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > @@ -88,6 +88,17 @@ __weak noinline struct file *bpf_testmod_return_ptr(int > arg) > } > } > > +struct testisra { > + int val1; > + int val2; > + int val3; > +}; > + > +static noinline void bpf_testmod_test_isra(struct testisra *t, int val1, > int val2) > +{ > + t->val3 = val1 + val2; > +} > + > noinline ssize_t > bpf_testmod_test_read(struct file *file, struct kobject *kobj, > struct bin_attribute *bin_attr, > @@ -98,8 +109,14 @@ __weak noinline struct file > *bpf_testmod_return_ptr(int arg) > .off = off, > .len = len, > }; > + struct testisra t = { > + .val1 = off, > + .val2 = len > + }; > int i = 1; > > + bpf_testmod_test_isra(&t, t.val1, t.val2); > + > while (bpf_testmod_return_ptr(i)) > i++; > > > Tested on gcc 9; possibly different results on different versions.. > > Alan Thanks for the pointer, will give it a try. Hengqi
On Tue, Mar 29, 2022 at 8:03 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > On 2022/3/30 10:50 AM, Andrii Nakryiko wrote: > > On Tue, Mar 29, 2022 at 7:30 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > >> > >> Hello, Andrii > >> > >> On 2022/3/30 7:18 AM, Andrii Nakryiko wrote: > >>> On Sat, Mar 26, 2022 at 7:43 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > >>>> > >>>> On some old kernels, kprobe auto-attach may fail when attach to symbols > >>>> like udp_send_skb.isra.52 . This is because the kernel has kprobe PMU > >>>> but don't allow attach to a symbol with '.' ([0]). Add a new option to > >>>> bpf_kprobe_opts to allow using the legacy kprobe attach directly. > >>>> This way, users can use bpf_program__attach_kprobe_opts in a dedicated > >>>> custom sec handler to handle such case. > >>>> > >>>> [0]: https://github.com/torvalds/linux/blob/v4.18/kernel/trace/trace_kprobe.c#L340-L343 > >>>> > >>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > >>>> --- > >>> > >>> It's sad, but it makes sense. But, let's have a selftests that > >>> validates uses legacy option explicitly (e.g., in > >>> prog_tests/attach_probe.c). Also, let's fix this limitation in the > >> > >> OK, will add a selftest to exercise the new option. > >> > >>> kernel? It makes no sense to limit attaching to a proper kallsym > >>> symbol. > >> > >> This limitation is lifted in newer kernel. Kernel v5.4 don't have this issue. > > > > Oh, ok. So how about another plan of attack then: if kprobe target > > function has '.' *and* we are on the kernel that doesn't support that, > > switch to legacy kprobe automatically? No need for a new option, > > libbpf handles this transparently. > > > > That's better, and also eliminate the need for custom SEC() handler. > > > Still need a test for kprobe with '.' in it, though not sure how > > reliable that will be... We can use kallsyms cache to check if > > expected xxx.isra.0 (or whatever) is present, and if not - skip > > subtest? > > > > Not sure how to do that. Even if such symbol exists, how to reliably > trigger it is another problem. In addition to what Alan proposed, which relies on compiler to do this whole isra thingy. I wonder if we can just create an alias symbol with dots in its name? I haven't tried, but would be curious to see if that works in bpf_testmod. > > >> > >>> > >>>> tools/lib/bpf/libbpf.c | 9 ++++++++- > >>>> tools/lib/bpf/libbpf.h | 4 +++- > >>>> 2 files changed, 11 insertions(+), 2 deletions(-) > >>>> > >>> > >>> [...] > >> > >> -- > >> Hengqi
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 809fe209cdcc..9a294bb84bad 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10097,9 +10097,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; snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx_%d", getpid(), kfunc_name, offset, __sync_fetch_and_add(&index, 1)); + + /* sanitize .isra.$n symbols */ + for (i = 0; buf[i]; i++) { + if (!isalnum(buf[i])) + buf[i] = '_'; + } } static int add_kprobe_event_legacy(const char *probe_name, bool retprobe, @@ -10189,7 +10196,7 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog, offset = OPTS_GET(opts, offset, 0); pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0); - legacy = determine_kprobe_perf_type() < 0; + legacy = OPTS_GET(opts, legacy, false) || determine_kprobe_perf_type() < 0; if (!legacy) { pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name, offset, diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 05dde85e19a6..88a3624e3f15 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -413,9 +413,11 @@ struct bpf_kprobe_opts { size_t offset; /* kprobe is return probe */ bool retprobe; + /* use the legacy debugfs interface */ + bool legacy; size_t :0; }; -#define bpf_kprobe_opts__last_field retprobe +#define bpf_kprobe_opts__last_field legacy LIBBPF_API struct bpf_link * bpf_program__attach_kprobe(const struct bpf_program *prog, bool retprobe,
On some old kernels, kprobe auto-attach may fail when attach to symbols like udp_send_skb.isra.52 . This is because the kernel has kprobe PMU but don't allow attach to a symbol with '.' ([0]). Add a new option to bpf_kprobe_opts to allow using the legacy kprobe attach directly. This way, users can use bpf_program__attach_kprobe_opts in a dedicated custom sec handler to handle such case. [0]: https://github.com/torvalds/linux/blob/v4.18/kernel/trace/trace_kprobe.c#L340-L343 Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- tools/lib/bpf/libbpf.c | 9 ++++++++- tools/lib/bpf/libbpf.h | 4 +++- 2 files changed, 11 insertions(+), 2 deletions(-) -- 2.30.2