Re: [PATCHv2 bpf-next 6/6] selftests/bpf: Fix get_func_ip offset test for CONFIG_X86_KERNEL_IBT

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Aug 15, 2022 at 08:25:51PM -0700, Andrii Nakryiko wrote:
> On Thu, Aug 11, 2022 at 2:16 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > With CONFIG_X86_KERNEL_IBT enabled the test for kprobe with offset
> > won't work because of the extra endbr instruction.
> >
> > As suggested by Andrii adding CONFIG_X86_KERNEL_IBT detection
> > and using appropriate offset value based on that.
> >
> > Also removing test7 program, because it does the same as test6.
> >
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../bpf/prog_tests/get_func_ip_test.c         | 62 +++++++++++++++----
> >  .../selftests/bpf/progs/get_func_ip_test.c    | 20 +++---
> >  2 files changed, 60 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> > index 938dbd4d7c2f..a4dab2fa2258 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> > @@ -2,7 +2,9 @@
> >  #include <test_progs.h>
> >  #include "get_func_ip_test.skel.h"
> >
> > -void test_get_func_ip_test(void)
> > +static int config_ibt;
> > +
> > +static void test_function_entry(void)
> >  {
> >         struct get_func_ip_test *skel = NULL;
> >         int err, prog_fd;
> > @@ -12,14 +14,6 @@ void test_get_func_ip_test(void)
> >         if (!ASSERT_OK_PTR(skel, "get_func_ip_test__open"))
> >                 return;
> >
> > -       /* test6 is x86_64 specifc because of the instruction
> > -        * offset, disabling it for all other archs
> > -        */
> > -#ifndef __x86_64__
> > -       bpf_program__set_autoload(skel->progs.test6, false);
> > -       bpf_program__set_autoload(skel->progs.test7, false);
> > -#endif
> > -
> >         err = get_func_ip_test__load(skel);
> >         if (!ASSERT_OK(err, "get_func_ip_test__load"))
> >                 goto cleanup;
> > @@ -38,16 +32,62 @@ void test_get_func_ip_test(void)
> >
> >         ASSERT_OK(err, "test_run");
> >
> > +       config_ibt = skel->bss->config_ibt;
> 
> skel->bss->config_ibt isn't actually necessary, you can just check
> skel->kconfig->CONFIG_X86_KERNEL_IBT directly. And you won't need to
> trigger BPF program unnecessary, libbpf will fill out kconfig section
> during object/skeleton load phase.

nice, did not know that ;-) will remove it

> 
> > +       ASSERT_TRUE(config_ibt == 0 || config_ibt == 1, "config_ibt");
> 
> you won't need this anymore
> 
> > +       printf("%s:config_ibt %d\n", __func__, config_ibt);
> 
> and this is just debug leftover

it's intentional to find out quickly what config we are failing on

> 
> > +
> >         ASSERT_EQ(skel->bss->test1_result, 1, "test1_result");
> >         ASSERT_EQ(skel->bss->test2_result, 1, "test2_result");
> >         ASSERT_EQ(skel->bss->test3_result, 1, "test3_result");
> >         ASSERT_EQ(skel->bss->test4_result, 1, "test4_result");
> >         ASSERT_EQ(skel->bss->test5_result, 1, "test5_result");
> > +
> 
> [...]
> 
> > 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 6db70757bc8b..cb8e58183d46 100644
> > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > @@ -2,6 +2,7 @@
> >  #include <linux/bpf.h>
> >  #include <bpf/bpf_helpers.h>
> >  #include <bpf/bpf_tracing.h>
> > +#include <stdbool.h>
> >
> >  char _license[] SEC("license") = "GPL";
> >
> > @@ -13,12 +14,19 @@ extern const void bpf_modify_return_test __ksym;
> >  extern const void bpf_fentry_test6 __ksym;
> >  extern const void bpf_fentry_test7 __ksym;
> >
> > +extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
> > +
> > +bool config_ibt;
> > +
> >  __u64 test1_result = 0;
> >  SEC("fentry/bpf_fentry_test1")
> >  int BPF_PROG(test1, int a)
> >  {
> >         __u64 addr = bpf_get_func_ip(ctx);
> >
> > +       /* just to propagate config option value to user space */
> > +       config_ibt = CONFIG_X86_KERNEL_IBT;
> > +
> 
> as mentioned above, you shouldn't need it, just read
> CONFIG_X86_KERNEL_IBT directly through skeleton
> 
> >         test1_result = (const void *) addr == &bpf_fentry_test1;
> >         return 0;
> >  }
> > @@ -64,7 +72,7 @@ int BPF_PROG(test5, int a, int *b, int ret)
> >  }
> >
> >  __u64 test6_result = 0;
> > -SEC("kprobe/bpf_fentry_test6+0x5")
> > +SEC("?kprobe/")
> 
> don't leave / at the end (and I thought that libbpf rejects this,
> isn't that a case?...), just SEC("?kprobe")

yes, will remove

thanks,
jirka