| Message ID | 20210707214751.159713-8-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf, x86: Add bpf_get_func_ip helper | expand |
On Wed, Jul 7, 2021 at 2:54 PM Jiri Olsa <jolsa@redhat.com> wrote: > > Adding test for bpf_get_func_ip in kprobe+ofset probe. typo: offset > Because of the offset value it's arch specific, adding > it only for x86_64 architecture. I'm not following, you specified +0x5 offset explicitly, why is this arch-specific? > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > .../testing/selftests/bpf/progs/get_func_ip_test.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > index 8ca54390d2b1..e8a9428a0ea3 100644 > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > @@ -10,6 +10,7 @@ extern const void bpf_fentry_test2 __ksym; > extern const void bpf_fentry_test3 __ksym; > extern const void bpf_fentry_test4 __ksym; > extern const void bpf_modify_return_test __ksym; > +extern const void bpf_fentry_test6 __ksym; > > __u64 test1_result = 0; > SEC("fentry/bpf_fentry_test1") > @@ -60,3 +61,15 @@ int BPF_PROG(fmod_ret_test, int a, int *b, int ret) > test5_result = (const void *) addr == &bpf_modify_return_test; > return ret; > } > + > +#ifdef __x86_64__ > +__u64 test6_result = 0; see, and you just forgot to update the user-space part of the test to even check test6_result... please group variables together and do explicit ASSERT_EQ > +SEC("kprobe/bpf_fentry_test6+0x5") > +int test6(struct pt_regs *ctx) > +{ > + __u64 addr = bpf_get_func_ip(ctx); > + > + test6_result = (const void *) addr == &bpf_fentry_test6 + 5; > + return 0; > +} > +#endif > -- > 2.31.1 >
On Wed, Jul 07, 2021 at 05:18:49PM -0700, Andrii Nakryiko wrote: > On Wed, Jul 7, 2021 at 2:54 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > Adding test for bpf_get_func_ip in kprobe+ofset probe. > > typo: offset > > > Because of the offset value it's arch specific, adding > > it only for x86_64 architecture. > > I'm not following, you specified +0x5 offset explicitly, why is this > arch-specific? I need some instruction offset != 0 in the traced function, x86_64's fentry jump is 5 bytes, other archs will be different > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > .../testing/selftests/bpf/progs/get_func_ip_test.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > index 8ca54390d2b1..e8a9428a0ea3 100644 > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > @@ -10,6 +10,7 @@ extern const void bpf_fentry_test2 __ksym; > > extern const void bpf_fentry_test3 __ksym; > > extern const void bpf_fentry_test4 __ksym; > > extern const void bpf_modify_return_test __ksym; > > +extern const void bpf_fentry_test6 __ksym; > > > > __u64 test1_result = 0; > > SEC("fentry/bpf_fentry_test1") > > @@ -60,3 +61,15 @@ int BPF_PROG(fmod_ret_test, int a, int *b, int ret) > > test5_result = (const void *) addr == &bpf_modify_return_test; > > return ret; > > } > > + > > +#ifdef __x86_64__ > > +__u64 test6_result = 0; > > see, and you just forgot to update the user-space part of the test to > even check test6_result... > > please group variables together and do explicit ASSERT_EQ right.. will change thanks, jirka > > > +SEC("kprobe/bpf_fentry_test6+0x5") > > +int test6(struct pt_regs *ctx) > > +{ > > + __u64 addr = bpf_get_func_ip(ctx); > > + > > + test6_result = (const void *) addr == &bpf_fentry_test6 + 5; > > + return 0; > > +} > > +#endif > > -- > > 2.31.1 > > >
On Sun, Jul 11, 2021 at 7:48 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Wed, Jul 07, 2021 at 05:18:49PM -0700, Andrii Nakryiko wrote: > > On Wed, Jul 7, 2021 at 2:54 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > Adding test for bpf_get_func_ip in kprobe+ofset probe. > > > > typo: offset > > > > > Because of the offset value it's arch specific, adding > > > it only for x86_64 architecture. > > > > I'm not following, you specified +0x5 offset explicitly, why is this > > arch-specific? > > I need some instruction offset != 0 in the traced function, > x86_64's fentry jump is 5 bytes, other archs will be different Right, ok. I don't see an easy way to detect this offset, but the #ifdef __x86_64__ detection doesn't work because we are compiling with -target bpf. Please double-check that it actually worked in the first place. I think a better way would be to have test6 defined unconditionally in BPF code, but then disable loading test6 program on anything but x86_64 platform at runtime with bpf_program__set_autoload(false). > > > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > --- > > > .../testing/selftests/bpf/progs/get_func_ip_test.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > > index 8ca54390d2b1..e8a9428a0ea3 100644 > > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > > @@ -10,6 +10,7 @@ extern const void bpf_fentry_test2 __ksym; > > > extern const void bpf_fentry_test3 __ksym; > > > extern const void bpf_fentry_test4 __ksym; > > > extern const void bpf_modify_return_test __ksym; > > > +extern const void bpf_fentry_test6 __ksym; > > > > > > __u64 test1_result = 0; > > > SEC("fentry/bpf_fentry_test1") > > > @@ -60,3 +61,15 @@ int BPF_PROG(fmod_ret_test, int a, int *b, int ret) > > > test5_result = (const void *) addr == &bpf_modify_return_test; > > > return ret; > > > } > > > + > > > +#ifdef __x86_64__ > > > +__u64 test6_result = 0; > > > > see, and you just forgot to update the user-space part of the test to > > even check test6_result... > > > > please group variables together and do explicit ASSERT_EQ > > right.. will change > > thanks, > jirka > > > > > > +SEC("kprobe/bpf_fentry_test6+0x5") > > > +int test6(struct pt_regs *ctx) > > > +{ > > > + __u64 addr = bpf_get_func_ip(ctx); > > > + > > > + test6_result = (const void *) addr == &bpf_fentry_test6 + 5; > > > + return 0; > > > +} > > > +#endif > > > -- > > > 2.31.1 > > > > > >
On Mon, Jul 12, 2021 at 04:32:25PM -0700, Andrii Nakryiko wrote: > On Sun, Jul 11, 2021 at 7:48 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Wed, Jul 07, 2021 at 05:18:49PM -0700, Andrii Nakryiko wrote: > > > On Wed, Jul 7, 2021 at 2:54 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > Adding test for bpf_get_func_ip in kprobe+ofset probe. > > > > > > typo: offset > > > > > > > Because of the offset value it's arch specific, adding > > > > it only for x86_64 architecture. > > > > > > I'm not following, you specified +0x5 offset explicitly, why is this > > > arch-specific? > > > > I need some instruction offset != 0 in the traced function, > > x86_64's fentry jump is 5 bytes, other archs will be different > > Right, ok. I don't see an easy way to detect this offset, but the > #ifdef __x86_64__ detection doesn't work because we are compiling with > -target bpf. Please double-check that it actually worked in the first > place. ugh, right > > I think a better way would be to have test6 defined unconditionally in > BPF code, but then disable loading test6 program on anything but > x86_64 platform at runtime with bpf_program__set_autoload(false). great, I did not know about this function, will be easier thanks, jirka > > > > > > > > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > > --- > > > > .../testing/selftests/bpf/progs/get_func_ip_test.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > > > index 8ca54390d2b1..e8a9428a0ea3 100644 > > > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > > > @@ -10,6 +10,7 @@ extern const void bpf_fentry_test2 __ksym; > > > > extern const void bpf_fentry_test3 __ksym; > > > > extern const void bpf_fentry_test4 __ksym; > > > > extern const void bpf_modify_return_test __ksym; > > > > +extern const void bpf_fentry_test6 __ksym; > > > > > > > > __u64 test1_result = 0; > > > > SEC("fentry/bpf_fentry_test1") > > > > @@ -60,3 +61,15 @@ int BPF_PROG(fmod_ret_test, int a, int *b, int ret) > > > > test5_result = (const void *) addr == &bpf_modify_return_test; > > > > return ret; > > > > } > > > > + > > > > +#ifdef __x86_64__ > > > > +__u64 test6_result = 0; > > > > > > see, and you just forgot to update the user-space part of the test to > > > even check test6_result... > > > > > > please group variables together and do explicit ASSERT_EQ > > > > right.. will change > > > > thanks, > > jirka > > > > > > > > > +SEC("kprobe/bpf_fentry_test6+0x5") > > > > +int test6(struct pt_regs *ctx) > > > > +{ > > > > + __u64 addr = bpf_get_func_ip(ctx); > > > > + > > > > + test6_result = (const void *) addr == &bpf_fentry_test6 + 5; > > > > + return 0; > > > > +} > > > > +#endif > > > > -- > > > > 2.31.1 > > > > > > > > > >
diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c index 8ca54390d2b1..e8a9428a0ea3 100644 --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c @@ -10,6 +10,7 @@ extern const void bpf_fentry_test2 __ksym; extern const void bpf_fentry_test3 __ksym; extern const void bpf_fentry_test4 __ksym; extern const void bpf_modify_return_test __ksym; +extern const void bpf_fentry_test6 __ksym; __u64 test1_result = 0; SEC("fentry/bpf_fentry_test1") @@ -60,3 +61,15 @@ int BPF_PROG(fmod_ret_test, int a, int *b, int ret) test5_result = (const void *) addr == &bpf_modify_return_test; return ret; } + +#ifdef __x86_64__ +__u64 test6_result = 0; +SEC("kprobe/bpf_fentry_test6+0x5") +int test6(struct pt_regs *ctx) +{ + __u64 addr = bpf_get_func_ip(ctx); + + test6_result = (const void *) addr == &bpf_fentry_test6 + 5; + return 0; +} +#endif
Adding test for bpf_get_func_ip in kprobe+ofset probe. Because of the offset value it's arch specific, adding it only for x86_64 architecture. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- .../testing/selftests/bpf/progs/get_func_ip_test.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)