On Mon, Apr 29, 2024 at 12:39 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Fri, Apr 26, 2024 at 11:06:53AM -0700, Andrii Nakryiko wrote:
> > On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > >
> > > Adding test that adds return uprobe inside 32 bit task
> > > and verify the return uprobe and attached bpf programs
> > > get properly executed.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > ---
> > > tools/testing/selftests/bpf/.gitignore | 1 +
> > > tools/testing/selftests/bpf/Makefile | 6 ++-
> > > .../selftests/bpf/prog_tests/uprobe_syscall.c | 40 +++++++++++++++++++
> > > .../bpf/progs/uprobe_syscall_compat.c | 13 ++++++
> > > 4 files changed, 59 insertions(+), 1 deletion(-)
> > > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> > > index f1aebabfb017..69d71223c0dd 100644
> > > --- a/tools/testing/selftests/bpf/.gitignore
> > > +++ b/tools/testing/selftests/bpf/.gitignore
> > > @@ -45,6 +45,7 @@ test_cpp
> > > /veristat
> > > /sign-file
> > > /uprobe_multi
> > > +/uprobe_compat
> > > *.ko
> > > *.tmp
> > > xskxceiver
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index edc73f8f5aef..d170b63eca62 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -134,7 +134,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
> > > xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> > > xdp_features bpf_test_no_cfi.ko
> > >
> > > -TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
> > > +TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi uprobe_compat
> >
> > you need to add uprobe_compat to TRUNNER_EXTRA_FILES as well, no?
>
> ah right
>
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > > index 9233210a4c33..3770254d893b 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > > @@ -11,6 +11,7 @@
> > > #include <sys/wait.h>
> > > #include "uprobe_syscall.skel.h"
> > > #include "uprobe_syscall_call.skel.h"
> > > +#include "uprobe_syscall_compat.skel.h"
> > >
> > > __naked unsigned long uretprobe_regs_trigger(void)
> > > {
> > > @@ -291,6 +292,35 @@ static void test_uretprobe_syscall_call(void)
> > > "read_trace_pipe_iter");
> > > ASSERT_EQ(found, 0, "found");
> > > }
> > > +
> > > +static void trace_pipe_compat_cb(const char *str, void *data)
> > > +{
> > > + if (strstr(str, "uretprobe compat") != NULL)
> > > + (*(int *)data)++;
> > > +}
> > > +
> > > +static void test_uretprobe_compat(void)
> > > +{
> > > + struct uprobe_syscall_compat *skel = NULL;
> > > + int err, found = 0;
> > > +
> > > + skel = uprobe_syscall_compat__open_and_load();
> > > + if (!ASSERT_OK_PTR(skel, "uprobe_syscall_compat__open_and_load"))
> > > + goto cleanup;
> > > +
> > > + err = uprobe_syscall_compat__attach(skel);
> > > + if (!ASSERT_OK(err, "uprobe_syscall_compat__attach"))
> > > + goto cleanup;
> > > +
> > > + system("./uprobe_compat");
> > > +
> > > + ASSERT_OK(read_trace_pipe_iter(trace_pipe_compat_cb, &found, 1000),
> > > + "read_trace_pipe_iter");
> >
> > why so complicated? can't you just set global variable that it was called
>
> hm, we execute separate uprobe_compat (32bit) process that triggers the bpf
> program, so we can't use global variable.. using the trace_pipe was the only
> thing that was easy to do
you need child process to trigger uprobe, but you could have installed
BPF program from parent process (you'd need to make child wait for
parent to be ready, with normal pipe() like we do in other places).
I think generally the less work forked child process does, the better.
All those ASSERT() failures won't produce any output in child process,
unless you run tests in verbose mode, because we haven't implemented
some form of sending all the logs back to the parent process and so
they are completely lost. But that's a separate topic.
Either way, consider using pipe() to coordinate waiting from child on
parent being ready, but otherwise do all the BPF-related heavy lifting
from parent (you can attach BPF programs to specific PID using
bpf_program__attach_uprobe() easily, it's not declarative, but simple
enough).
>
> jirka
>
> >
> > > + ASSERT_EQ(found, 1, "found");
> > > +
> > > +cleanup:
> > > + uprobe_syscall_compat__destroy(skel);
> > > +}
> > > #else
> > > static void test_uretprobe_regs_equal(void)
> > > {
> > > @@ -306,6 +336,11 @@ static void test_uretprobe_syscall_call(void)
> > > {
> > > test__skip();
> > > }
> > > +
> > > +static void test_uretprobe_compat(void)
> > > +{
> > > + test__skip();
> > > +}
> > > #endif
> > >
> > > void test_uprobe_syscall(void)
> > > @@ -320,3 +355,8 @@ void serial_test_uprobe_syscall_call(void)
> > > {
> > > test_uretprobe_syscall_call();
> > > }
> > > +
> > > +void serial_test_uprobe_syscall_compat(void)
> >
> > and then no need for serial_test?
> >
> > > +{
> > > + test_uretprobe_compat();
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > > new file mode 100644
> > > index 000000000000..f8adde7f08e2
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > > @@ -0,0 +1,13 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <linux/bpf.h>
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_tracing.h>
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > +
> > > +SEC("uretprobe.multi/./uprobe_compat:main")
> > > +int uretprobe_compat(struct pt_regs *ctx)
> > > +{
> > > + bpf_printk("uretprobe compat\n");
> > > + return 0;
> > > +}
> > > --
> > > 2.44.0
> > >
