On Tue, May 21, 2024 at 3:04 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Mon, May 20, 2024 at 09:54:40PM -0700, Andrii Nakryiko wrote:
> > On Mon, May 20, 2024 at 4:47 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > >
> > > Validate libbpf's USDT-over-multi-uprobe logic by adding USDTs to
> > > existing multi-uprobe tests. This checks correct libbpf fallback to
> > > singular uprobes (when run on older kernels with buggy PID filtering).
> > > We reuse already established child process and child thread testing
> > > infrastructure, so additions are minimal. These test fail on either
> > > older kernels or older version of libbpf that doesn't detect PID
> > > filtering problems.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > ---
> > > .../bpf/prog_tests/uprobe_multi_test.c | 22 +++++++++++++
> > > .../selftests/bpf/progs/uprobe_multi.c | 33 +++++++++++++++++--
> > > 2 files changed, 53 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > > index 677232d31432..85d46e568e90 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > > @@ -8,6 +8,7 @@
> > > #include "uprobe_multi_usdt.skel.h"
> > > #include "bpf/libbpf_internal.h"
> > > #include "testing_helpers.h"
> > > +#include "../sdt.h"
> > >
> > > static char test_data[] = "test_data";
> > >
> > > @@ -26,6 +27,11 @@ noinline void uprobe_multi_func_3(void)
> > > asm volatile ("");
> > > }
> > >
> > > +noinline void usdt_trigger(void)
> > > +{
> > > + STAP_PROBE(test, pid_filter_usdt);
> > > +}
> > > +
> > > struct child {
> > > int go[2];
> > > int c2p[2]; /* child -> parent channel */
> > > @@ -269,8 +275,24 @@ __test_attach_api(const char *binary, const char *pattern, struct bpf_uprobe_mul
> > > if (!ASSERT_OK_PTR(skel->links.uprobe_extra, "bpf_program__attach_uprobe_multi"))
> > > goto cleanup;
> > >
> > > + /* Attach (uprobe-backed) USDTs */
> > > + skel->links.usdt_pid = bpf_program__attach_usdt(skel->progs.usdt_pid, pid, binary,
> > > + "test", "pid_filter_usdt", NULL);
> > > + if (!ASSERT_OK_PTR(skel->links.usdt_pid, "attach_usdt_pid"))
> > > + goto cleanup;
> > > +
> > > + skel->links.usdt_extra = bpf_program__attach_usdt(skel->progs.usdt_extra, -1, binary,
> > > + "test", "pid_filter_usdt", NULL);
> > > + if (!ASSERT_OK_PTR(skel->links.usdt_extra, "attach_usdt_extra"))
> > > + goto cleanup;
> > > +
> > > uprobe_multi_test_run(skel, child);
> > >
> > > + ASSERT_FALSE(skel->bss->bad_pid_seen_usdt, "bad_pid_seen_usdt");
> > > + if (child) {
> > > + ASSERT_EQ(skel->bss->child_pid_usdt, child->pid, "usdt_multi_child_pid");
> > > + ASSERT_EQ(skel->bss->child_tid_usdt, child->tid, "usdt_multi_child_tid");
> > > + }
> > > cleanup:
> > > uprobe_multi__destroy(skel);
> > > }
> > > diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi.c b/tools/testing/selftests/bpf/progs/uprobe_multi.c
> > > index 86a7ff5d3726..44190efcdba2 100644
> > > --- a/tools/testing/selftests/bpf/progs/uprobe_multi.c
> > > +++ b/tools/testing/selftests/bpf/progs/uprobe_multi.c
> > > @@ -1,8 +1,8 @@
> > > // SPDX-License-Identifier: GPL-2.0
> > > -#include <linux/bpf.h>
> > > +#include "vmlinux.h"
> > > #include <bpf/bpf_helpers.h>
> > > #include <bpf/bpf_tracing.h>
> > > -#include <stdbool.h>
> > > +#include <bpf/usdt.bpf.h>
> > >
> > > char _license[] SEC("license") = "GPL";
> > >
> > > @@ -23,9 +23,12 @@ __u64 uprobe_multi_sleep_result = 0;
> > > int pid = 0;
> > > int child_pid = 0;
> > > int child_tid = 0;
> > > +int child_pid_usdt = 0;
> > > +int child_tid_usdt = 0;
> > >
> > > int expect_pid = 0;
> > > bool bad_pid_seen = false;
> > > +bool bad_pid_seen_usdt = false;
> > >
> > > bool test_cookie = false;
> > > void *user_ptr = 0;
> > > @@ -112,3 +115,29 @@ int uprobe_extra(struct pt_regs *ctx)
> > > /* we need this one just to mix PID-filtered and global uprobes */
> > > return 0;
> > > }
> > > +
> > > +SEC("usdt")
> > > +int usdt_pid(struct pt_regs *ctx)
> > > +{
> > > + __u64 cur_pid_tgid = bpf_get_current_pid_tgid();
> > > + __u32 cur_pid;
> > > +
> > > + cur_pid = cur_pid_tgid >> 32;
> > > + if (pid && cur_pid != pid)
> > > + return 0;
> > > +
> > > + if (expect_pid && cur_pid != expect_pid)
> > > + bad_pid_seen_usdt = true;
> > > +
> > > + child_pid_usdt = cur_pid_tgid >> 32;
> > > + child_tid_usdt = (__u32)cur_pid_tgid;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +SEC("usdt")
> > > +int usdt_extra(struct pt_regs *ctx)
> > > +{
> > > + /* we need this one just to mix PID-filtered and global USDT probes */
> > > + return 0;
> > > +}
> > > --
> > > 2.43.0
> > >
> >
> > I lost the following during the final rebase before submitting,
> > sigh... With the piece below tests are passing again:
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > index 85d46e568e90..bf6ca8e3eb13 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > @@ -96,6 +96,7 @@ static struct child *spawn_child(void)
> > uprobe_multi_func_1();
> > uprobe_multi_func_2();
> > uprobe_multi_func_3();
> > + usdt_trigger();
> >
> > exit(errno);
> > }
> > @@ -123,6 +124,7 @@ static void *child_thread(void *ctx)
> > uprobe_multi_func_1();
> > uprobe_multi_func_2();
> > uprobe_multi_func_3();
> > + usdt_trigger();
> >
> > err = 0;
> > pthread_exit(&err);
> > @@ -188,6 +190,7 @@ static void uprobe_multi_test_run(struct
> > uprobe_multi *skel, struct child *child
> > uprobe_multi_func_1();
> > uprobe_multi_func_2();
> > uprobe_multi_func_3();
> > + usdt_trigger();
> > }
> >
> > if (child)
> >
> >
> > I'll wait till tomorrow for any feedback and will post v2.
>
> tests are passing for me with the changes above
>
> >
> > I'm also curious about logistics? Do we want to get everything through
> > the bpf tree? Or bpf-next? Or split somehow? Thoughts?
> >
> > I think the fix in patch #1 is important enough to backport to stable
> > kernels (multi-uprobes went into upstream v6.6 kernel, FYI).
>
> agreed, perhaps also the patch #3 for libbpf detection?
no one should be building libbpf from kernel sources, Github is the
authoritative source code for packaging. So I think it's not necessary
to backport libbpf. Tests will keep passing with just patch #1.
>
> jirka
