| Message ID | 20211006185619.364369-10-fallentree@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | selftests/bpf: Add parallelism to test_progs | expand |
On Wed, Oct 6, 2021 at 11:56 AM Yucong Sun <fallentree@fb.com> wrote: > > From: Yucong Sun <sunyucong@gmail.com> > > Using same address on different processes of the same binary often fail > with EINVAL, this patch make these tests use distinct methods, so they > can run in parallel. > > Signed-off-by: Yucong Sun <sunyucong@gmail.com> > --- > tools/testing/selftests/bpf/prog_tests/attach_probe.c | 8 ++++++-- > tools/testing/selftests/bpf/prog_tests/bpf_cookie.c | 8 ++++++-- > tools/testing/selftests/bpf/prog_tests/task_pt_regs.c | 8 ++++++-- > 3 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c > index 6c511dcd1465..eff36ba9c148 100644 > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c > @@ -5,6 +5,10 @@ > /* this is how USDT semaphore is actually defined, except volatile modifier */ > volatile unsigned short uprobe_ref_ctr __attribute__((unused)) __attribute((section(".probes"))); > > +static int method() { wrong style: { should be on separate line > + return get_base_addr(); there is nothing special about get_base_addr(), except that it's a global function in a different file and won't be inlined, while this method() approach has no such guarantee I've dropped this patch for now. But I'm surprised that attaching to the same uprobe few times doesn't work. Song, is there anything in kernel that could cause this? > +} > + > void test_attach_probe(void) > { > DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts); > @@ -33,7 +37,7 @@ void test_attach_probe(void) > if (CHECK(base_addr < 0, "get_base_addr", > "failed to find base addr: %zd", base_addr)) > return; > - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); > + uprobe_offset = get_uprobe_offset(&method, base_addr); > > ref_ctr_offset = get_rel_offset((uintptr_t)&uprobe_ref_ctr); > if (!ASSERT_GE(ref_ctr_offset, 0, "ref_ctr_offset")) > @@ -98,7 +102,7 @@ void test_attach_probe(void) > goto cleanup; > > /* trigger & validate uprobe & uretprobe */ > - get_base_addr(); > + method(); > > if (CHECK(skel->bss->uprobe_res != 3, "check_uprobe_res", > "wrong uprobe res: %d\n", skel->bss->uprobe_res)) > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > index 19c9f7b53cfa..5ebd8ba988e2 100644 > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > @@ -8,6 +8,10 @@ > #include <test_progs.h> > #include "test_bpf_cookie.skel.h" > > +static int method() { > + return get_base_addr(); > +} > + > static void kprobe_subtest(struct test_bpf_cookie *skel) > { > DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts); > @@ -66,7 +70,7 @@ static void uprobe_subtest(struct test_bpf_cookie *skel) > ssize_t base_addr; > > base_addr = get_base_addr(); > - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); > + uprobe_offset = get_uprobe_offset(&method, base_addr); > > /* attach two uprobes */ > opts.bpf_cookie = 0x100; > @@ -99,7 +103,7 @@ static void uprobe_subtest(struct test_bpf_cookie *skel) > goto cleanup; > > /* trigger uprobe && uretprobe */ > - get_base_addr(); > + method(); > > ASSERT_EQ(skel->bss->uprobe_res, 0x100 | 0x200, "uprobe_res"); > ASSERT_EQ(skel->bss->uretprobe_res, 0x1000 | 0x2000, "uretprobe_res"); > diff --git a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c > index 37c20b5ffa70..4d2f1435be90 100644 > --- a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c > +++ b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c > @@ -3,6 +3,10 @@ > #include <test_progs.h> > #include "test_task_pt_regs.skel.h" > > +static int method() { > + return get_base_addr(); > +} > + > void test_task_pt_regs(void) > { > struct test_task_pt_regs *skel; > @@ -14,7 +18,7 @@ void test_task_pt_regs(void) > base_addr = get_base_addr(); > if (!ASSERT_GT(base_addr, 0, "get_base_addr")) > return; > - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); > + uprobe_offset = get_uprobe_offset(&method, base_addr); > > skel = test_task_pt_regs__open_and_load(); > if (!ASSERT_OK_PTR(skel, "skel_open")) > @@ -32,7 +36,7 @@ void test_task_pt_regs(void) > skel->links.handle_uprobe = uprobe_link; > > /* trigger & validate uprobe */ > - get_base_addr(); > + method(); > > if (!ASSERT_EQ(skel->bss->uprobe_res, 1, "check_uprobe_res")) > goto cleanup; > -- > 2.30.2 >
On Fri, Oct 8, 2021 at 3:27 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Oct 6, 2021 at 11:56 AM Yucong Sun <fallentree@fb.com> wrote: > > > > From: Yucong Sun <sunyucong@gmail.com> > > > > Using same address on different processes of the same binary often fail > > with EINVAL, this patch make these tests use distinct methods, so they > > can run in parallel. > > > > Signed-off-by: Yucong Sun <sunyucong@gmail.com> > > --- > > tools/testing/selftests/bpf/prog_tests/attach_probe.c | 8 ++++++-- > > tools/testing/selftests/bpf/prog_tests/bpf_cookie.c | 8 ++++++-- > > tools/testing/selftests/bpf/prog_tests/task_pt_regs.c | 8 ++++++-- > > 3 files changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c > > index 6c511dcd1465..eff36ba9c148 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c > > @@ -5,6 +5,10 @@ > > /* this is how USDT semaphore is actually defined, except volatile modifier */ > > volatile unsigned short uprobe_ref_ctr __attribute__((unused)) __attribute((section(".probes"))); > > > > +static int method() { > > wrong style: { should be on separate line > > > + return get_base_addr(); > > there is nothing special about get_base_addr(), except that it's a > global function in a different file and won't be inlined, while this > method() approach has no such guarantee > > I've dropped this patch for now. > > But I'm surprised that attaching to the same uprobe few times doesn't > work. Song, is there anything in kernel that could cause this? libbpf: uprobe perf_event_open() failed: Invalid argument libbpf: prog 'handle_uprobe': failed to create uprobe '/proc/self/exe:0x144d59' perf event: Invalid argument uprobe_subtest:FAIL:link1 unexpected error: -22 The problem only happens when several different processes of the same binary are trying to attach uprobe on the same function. I am guessing it is due to address space randomization ? I traced through the code and the EINVAL is returned right after this warning [ 1.375901] ref_ctr_offset mismatch. inode: 0x55a0 offset: 0x144d59 ref_ctr_offset(old): 0x554a00 ref_ctr_offset(new): 0x0 This could be easily be reproduced by ./test_progs -t attach_probe,bpf_cookie,test_pt_regs -j > > > > +} > > + > > void test_attach_probe(void) > > { > > DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts); > > @@ -33,7 +37,7 @@ void test_attach_probe(void) > > if (CHECK(base_addr < 0, "get_base_addr", > > "failed to find base addr: %zd", base_addr)) > > return; > > - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); > > + uprobe_offset = get_uprobe_offset(&method, base_addr); > > > > ref_ctr_offset = get_rel_offset((uintptr_t)&uprobe_ref_ctr); > > if (!ASSERT_GE(ref_ctr_offset, 0, "ref_ctr_offset")) > > @@ -98,7 +102,7 @@ void test_attach_probe(void) > > goto cleanup; > > > > /* trigger & validate uprobe & uretprobe */ > > - get_base_addr(); > > + method(); > > > > if (CHECK(skel->bss->uprobe_res != 3, "check_uprobe_res", > > "wrong uprobe res: %d\n", skel->bss->uprobe_res)) > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > index 19c9f7b53cfa..5ebd8ba988e2 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > @@ -8,6 +8,10 @@ > > #include <test_progs.h> > > #include "test_bpf_cookie.skel.h" > > > > +static int method() { > > + return get_base_addr(); > > +} > > + > > static void kprobe_subtest(struct test_bpf_cookie *skel) > > { > > DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts); > > @@ -66,7 +70,7 @@ static void uprobe_subtest(struct test_bpf_cookie *skel) > > ssize_t base_addr; > > > > base_addr = get_base_addr(); > > - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); > > + uprobe_offset = get_uprobe_offset(&method, base_addr); > > > > /* attach two uprobes */ > > opts.bpf_cookie = 0x100; > > @@ -99,7 +103,7 @@ static void uprobe_subtest(struct test_bpf_cookie *skel) > > goto cleanup; > > > > /* trigger uprobe && uretprobe */ > > - get_base_addr(); > > + method(); > > > > ASSERT_EQ(skel->bss->uprobe_res, 0x100 | 0x200, "uprobe_res"); > > ASSERT_EQ(skel->bss->uretprobe_res, 0x1000 | 0x2000, "uretprobe_res"); > > diff --git a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c > > index 37c20b5ffa70..4d2f1435be90 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c > > +++ b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c > > @@ -3,6 +3,10 @@ > > #include <test_progs.h> > > #include "test_task_pt_regs.skel.h" > > > > +static int method() { > > + return get_base_addr(); > > +} > > + > > void test_task_pt_regs(void) > > { > > struct test_task_pt_regs *skel; > > @@ -14,7 +18,7 @@ void test_task_pt_regs(void) > > base_addr = get_base_addr(); > > if (!ASSERT_GT(base_addr, 0, "get_base_addr")) > > return; > > - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); > > + uprobe_offset = get_uprobe_offset(&method, base_addr); > > > > skel = test_task_pt_regs__open_and_load(); > > if (!ASSERT_OK_PTR(skel, "skel_open")) > > @@ -32,7 +36,7 @@ void test_task_pt_regs(void) > > skel->links.handle_uprobe = uprobe_link; > > > > /* trigger & validate uprobe */ > > - get_base_addr(); > > + method(); > > > > if (!ASSERT_EQ(skel->bss->uprobe_res, 1, "check_uprobe_res")) > > goto cleanup; > > -- > > 2.30.2 > >
On Fri, Oct 8, 2021 at 3:47 PM sunyucong@gmail.com <sunyucong@gmail.com> wrote: > > On Fri, Oct 8, 2021 at 3:27 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Oct 6, 2021 at 11:56 AM Yucong Sun <fallentree@fb.com> wrote: > > > > > > From: Yucong Sun <sunyucong@gmail.com> > > > > > > Using same address on different processes of the same binary often fail > > > with EINVAL, this patch make these tests use distinct methods, so they > > > can run in parallel. > > > > > > Signed-off-by: Yucong Sun <sunyucong@gmail.com> > > > --- > > > tools/testing/selftests/bpf/prog_tests/attach_probe.c | 8 ++++++-- > > > tools/testing/selftests/bpf/prog_tests/bpf_cookie.c | 8 ++++++-- > > > tools/testing/selftests/bpf/prog_tests/task_pt_regs.c | 8 ++++++-- > > > 3 files changed, 18 insertions(+), 6 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c > > > index 6c511dcd1465..eff36ba9c148 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c > > > @@ -5,6 +5,10 @@ > > > /* this is how USDT semaphore is actually defined, except volatile modifier */ > > > volatile unsigned short uprobe_ref_ctr __attribute__((unused)) __attribute((section(".probes"))); > > > > > > +static int method() { > > > > wrong style: { should be on separate line > > > > > + return get_base_addr(); > > > > there is nothing special about get_base_addr(), except that it's a > > global function in a different file and won't be inlined, while this > > method() approach has no such guarantee > > > > I've dropped this patch for now. > > > > But I'm surprised that attaching to the same uprobe few times doesn't > > work. Song, is there anything in kernel that could cause this? > > > libbpf: uprobe perf_event_open() failed: Invalid argument > libbpf: prog 'handle_uprobe': failed to create uprobe > '/proc/self/exe:0x144d59' perf event: Invalid argument > uprobe_subtest:FAIL:link1 unexpected error: -22 > > The problem only happens when several different processes of the same > binary are trying to attach uprobe on the same function. I am guessing > it is due to address space randomization ? nope, we don't use address space randomization, it's the ref_ctr_offset (normally used for USDT semaphore) > > I traced through the code and the EINVAL is returned right after this warning > > [ 1.375901] ref_ctr_offset mismatch. inode: 0x55a0 offset: 0x144d59 > ref_ctr_offset(old): 0x554a00 ref_ctr_offset(new): 0x0 ah, ref_ctr_offset is probably enforced by the kernel to be the same across all uprobes. attach_probe is the only one that's testing ref_ctr_offset, so it should be enough to modify only that one, just make sure you are using a non-inlined function for this. > > > This could be easily be reproduced by ./test_progs -t > attach_probe,bpf_cookie,test_pt_regs -j > > > > > > > > +} > > > + [...]
diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c index 6c511dcd1465..eff36ba9c148 100644 --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c @@ -5,6 +5,10 @@ /* this is how USDT semaphore is actually defined, except volatile modifier */ volatile unsigned short uprobe_ref_ctr __attribute__((unused)) __attribute((section(".probes"))); +static int method() { + return get_base_addr(); +} + void test_attach_probe(void) { DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts); @@ -33,7 +37,7 @@ void test_attach_probe(void) if (CHECK(base_addr < 0, "get_base_addr", "failed to find base addr: %zd", base_addr)) return; - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); + uprobe_offset = get_uprobe_offset(&method, base_addr); ref_ctr_offset = get_rel_offset((uintptr_t)&uprobe_ref_ctr); if (!ASSERT_GE(ref_ctr_offset, 0, "ref_ctr_offset")) @@ -98,7 +102,7 @@ void test_attach_probe(void) goto cleanup; /* trigger & validate uprobe & uretprobe */ - get_base_addr(); + method(); if (CHECK(skel->bss->uprobe_res != 3, "check_uprobe_res", "wrong uprobe res: %d\n", skel->bss->uprobe_res)) diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c index 19c9f7b53cfa..5ebd8ba988e2 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c @@ -8,6 +8,10 @@ #include <test_progs.h> #include "test_bpf_cookie.skel.h" +static int method() { + return get_base_addr(); +} + static void kprobe_subtest(struct test_bpf_cookie *skel) { DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts); @@ -66,7 +70,7 @@ static void uprobe_subtest(struct test_bpf_cookie *skel) ssize_t base_addr; base_addr = get_base_addr(); - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); + uprobe_offset = get_uprobe_offset(&method, base_addr); /* attach two uprobes */ opts.bpf_cookie = 0x100; @@ -99,7 +103,7 @@ static void uprobe_subtest(struct test_bpf_cookie *skel) goto cleanup; /* trigger uprobe && uretprobe */ - get_base_addr(); + method(); ASSERT_EQ(skel->bss->uprobe_res, 0x100 | 0x200, "uprobe_res"); ASSERT_EQ(skel->bss->uretprobe_res, 0x1000 | 0x2000, "uretprobe_res"); diff --git a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c index 37c20b5ffa70..4d2f1435be90 100644 --- a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c +++ b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c @@ -3,6 +3,10 @@ #include <test_progs.h> #include "test_task_pt_regs.skel.h" +static int method() { + return get_base_addr(); +} + void test_task_pt_regs(void) { struct test_task_pt_regs *skel; @@ -14,7 +18,7 @@ void test_task_pt_regs(void) base_addr = get_base_addr(); if (!ASSERT_GT(base_addr, 0, "get_base_addr")) return; - uprobe_offset = get_uprobe_offset(&get_base_addr, base_addr); + uprobe_offset = get_uprobe_offset(&method, base_addr); skel = test_task_pt_regs__open_and_load(); if (!ASSERT_OK_PTR(skel, "skel_open")) @@ -32,7 +36,7 @@ void test_task_pt_regs(void) skel->links.handle_uprobe = uprobe_link; /* trigger & validate uprobe */ - get_base_addr(); + method(); if (!ASSERT_EQ(skel->bss->uprobe_res, 1, "check_uprobe_res")) goto cleanup;