[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-api
Subject:    Re: [PATCHv3 bpf-next 6/7] selftests/bpf: Add uretprobe compat test
From:       Andrii Nakryiko <andrii.nakryiko () gmail ! com>
Date:       2024-04-29 16:44:49
Message-ID: CAEf4Bzb6M=8x6=WMZq8GDB3gS4kejsB9QaVSNEc1MwWbX_vvUQ () mail ! gmail ! com
[Download RAW message or body]

On Mon, Apr 29, 2024 at 12:39 AM Jiri Olsa <olsajiri@gmail.com> 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@kernel.org> 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@kernel.org>
> > > ---
> > > 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
> > > 


[prev in list] [next in list] [prev in thread] [next in thread]