linux-kernel - Re: [PATCHv2 bpf-next 9/9] selftests/bpf: Add uprobe session consumers test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZoWIzZzZaqNR6dLm@krava>
Date: Wed, 3 Jul 2024 19:22:21 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Oleg Nesterov <oleg@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
	Martin KaFai Lau <kafai@...com>, Song Liu <songliubraving@...com>,
	Yonghong Song <yhs@...com>,
	John Fastabend <john.fastabend@...il.com>,
	KP Singh <kpsingh@...omium.org>,
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCHv2 bpf-next 9/9] selftests/bpf: Add uprobe session
 consumers test

On Tue, Jul 02, 2024 at 03:10:55PM -0700, Andrii Nakryiko wrote:
> On Mon, Jul 1, 2024 at 9:44 AM Jiri Olsa <jolsa@...nel.org> wrote:
> >
> > Adding test that attached/detaches multiple consumers on
> > single uprobe and verifies all were hit as expected.
> >
> > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  .../bpf/prog_tests/uprobe_multi_test.c        | 203 ++++++++++++++++++
> >  .../progs/uprobe_multi_session_consumers.c    |  53 +++++
> >  2 files changed, 256 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session_consumers.c
> >
> 
> This is clever, though bit notation obscures the meaning of the code a
> bit. But thanks for the long comment explaining the overall idea.
> 
> > 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 b521590fdbb9..83eac954cf00 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > @@ -9,6 +9,7 @@
> >  #include "uprobe_multi_session.skel.h"
> >  #include "uprobe_multi_session_cookie.skel.h"
> >  #include "uprobe_multi_session_recursive.skel.h"
> > +#include "uprobe_multi_session_consumers.skel.h"
> >  #include "bpf/libbpf_internal.h"
> >  #include "testing_helpers.h"
> >  #include "../sdt.h"
> > @@ -739,6 +740,206 @@ static void test_session_recursive_skel_api(void)
> >         uprobe_multi_session_recursive__destroy(skel);
> >  }
> >
> > +static int uprobe_attach(struct uprobe_multi_session_consumers *skel, int bit)
> > +{
> > +       struct bpf_program **prog = &skel->progs.uprobe_0 + bit;
> > +       struct bpf_link **link = &skel->links.uprobe_0 + bit;
> > +       LIBBPF_OPTS(bpf_uprobe_multi_opts, opts);
> > +
> > +       /*
> > +        * bit: 0,1 uprobe session
> > +        * bit: 2,3 uprobe entry
> > +        * bit: 4,5 uprobe return
> > +        */
> > +       opts.session = bit < 2;
> > +       opts.retprobe = bit == 4 || bit == 5;
> > +
> > +       *link = bpf_program__attach_uprobe_multi(*prog, 0, "/proc/self/exe",
> > +                                                "uprobe_session_consumer_test",
> > +                                                &opts);
> > +       if (!ASSERT_OK_PTR(*link, "bpf_program__attach_uprobe_multi"))
> > +               return -1;
> > +       return 0;
> > +}
> > +
> > +static void uprobe_detach(struct uprobe_multi_session_consumers *skel, int bit)
> > +{
> > +       struct bpf_link **link = &skel->links.uprobe_0 + bit;
> 
> ok, this is nasty, no one guarantees this should keep working,
> explicit switch would be preferable

I see, ok, will replace that with a switch

> 
> > +
> > +       bpf_link__destroy(*link);
> > +       *link = NULL;
> > +}
> > +
> > +static bool test_bit(int bit, unsigned long val)
> > +{
> > +       return val & (1 << bit);
> > +}
> > +
> > +noinline int
> > +uprobe_session_consumer_test(struct uprobe_multi_session_consumers *skel,
> > +                            unsigned long before, unsigned long after)
> > +{
> > +       int bit;
> > +
> > +       /* detach uprobe for each unset bit in 'before' state ... */
> > +       for (bit = 0; bit < 6; bit++) {
> 
> Does "bit" correspond to the uprobe_X program? Maybe call it an uprobe
> index or something, if that's the case? bits are just representations,
> but semantically meaningful is identifier of an uprobe program, right?

right.. so it corresponds to program 'uprobe_<bit>' so maybe 'idx' is better

thanks,
jirka