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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
> > .../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
