Received-SPF: pass (google.com: domain of linux-kernel+bounces-161869-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Jiri Olsa <olsajiri@gmail.com>
Date: Mon, 29 Apr 2024 09:39:44 +0200
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>, linux-kernel@vger.kernel.org,
	linux-trace-kernel@vger.kernel.org, linux-api@vger.kernel.org,
	x86@kernel.org, bpf@vger.kernel.org,
	Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Borislav Petkov (AMD)" <bp@alien8.de>,
	Ingo Molnar <mingo@redhat.com>, Andy Lutomirski <luto@kernel.org>
Subject: Re: [PATCHv3 bpf-next 6/7] selftests/bpf: Add uretprobe compat test
Message-ID: <Zi9OwCwluxTo-Azd@krava>
References: <20240421194206.1010934-1-jolsa@kernel.org>
 <20240421194206.1010934-7-jolsa@kernel.org>
 <CAEf4BzYU-y+vptqXpuALYecJJgPt+CTcbo+=Q9QXnu4vNwem+g@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEf4BzYU-y+vptqXpuALYecJJgPt+CTcbo+=Q9QXnu4vNwem+g@mail.gmail.com>

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

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
> >