Re: [PATCHv2 bpf-next 2/2] selftests/bpf: Add uprobe multi consumers test

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jul 19, 2024 at 10:58:07AM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 18, 2024 at 6:28 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test that attached/detaches multiple consumers on
> 
> typo: attaches
> 
> > single uprobe and verifies all were hit as expected.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../bpf/prog_tests/uprobe_multi_test.c        | 211 +++++++++++++++++-
> >  .../bpf/progs/uprobe_multi_consumers.c        |  39 ++++
> >  2 files changed, 249 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_consumers.c
> >
> 
> LGTM, took me a bit of extra time to validate the counting logic, but
> it looks correct.
> 
> > 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 da8873f24a53..5228085c2240 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > @@ -6,6 +6,7 @@
> >  #include "uprobe_multi.skel.h"
> >  #include "uprobe_multi_bench.skel.h"
> >  #include "uprobe_multi_usdt.skel.h"
> > +#include "uprobe_multi_consumers.skel.h"
> >  #include "bpf/libbpf_internal.h"
> >  #include "testing_helpers.h"
> >  #include "../sdt.h"
> > @@ -581,7 +582,7 @@ static void attach_uprobe_fail_refctr(struct uprobe_multi *skel)
> >                 goto cleanup;
> >
> >         /*
> > -        * We attach to 3 uprobes on 2 functions so 2 uprobes share single function,
> > +        * We attach to 3 uprobes on 2 functions, so 2 uprobes share single function,
> 
> this probably belongs in patch #1

ugh yep

SNIP

> > +static int uprobe_attach(struct uprobe_multi_consumers *skel, int idx)
> > +{
> > +       struct bpf_program *prog = get_program(skel, idx);
> > +       struct bpf_link **link = get_link(skel, idx);
> > +       LIBBPF_OPTS(bpf_uprobe_multi_opts, opts);
> > +
> > +       /*
> > +        * bit/prog: 0,1 uprobe entry
> > +        * bit/prog: 2,3 uprobe return
> > +        */
> > +       opts.retprobe = idx == 2 || idx == 3;
> > +
> > +       *link = bpf_program__attach_uprobe_multi(prog, 0, "/proc/self/exe",
> 
> 
> this will crash if idx is wrong, let's add explicit NULL checks for
> link and prog, just to fail gracefully?

ok

> 
> 
> > +                                               "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_consumers *skel, int idx)
> > +{
> > +       struct bpf_link **link = get_link(skel, idx);
> > +
> > +       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_consumers *skel,
> 
> this gave me pause, I was frantically recalling when did we end up
> landing uprobe sessions support :)

rename leftover sry ;-)


SNIP

> > +               } else {
> > +                       /* uprobe return is tricky ;-)
> > +                        *
> > +                        * to trigger uretprobe consumer, the uretprobe needs to be installed,
> > +                        * which means one of the 'return' uprobes was alive when probe was hit:
> > +                        *
> > +                        *   idxs: 2/3 uprobe return in 'installed' mask
> > +                        *
> > +                        * in addition if 'after' state removes everything that was installed in
> > +                        * 'before' state, then uprobe kernel object goes away and return uprobe
> > +                        * is not installed and we won't hit it even if it's in 'after' state.
> > +                        */
> 
> yeah, this is tricky, thanks for writing this out, seems correct to me
> 
> > +                       unsigned long installed = before & 0b1100; // is uretprobe installed
> > +                       unsigned long exists    = before & after;  // did uprobe go away
> > +
> > +                       if (installed && exists && test_bit(idx, after))
> 
> nit: naming didn't really help (actually probably hurt the analysis).
> installed is whether we had any uretprobes, so "had_uretprobes"?
> exists is whether uprobe stayed attached during function call, right,
> so maybe "probe_preserved" or something like that?
> 
> I.e., the condition should say "if we had any uretprobes, and the
> probe instance stayed alive, and the program is still attached at
> return".

yep, looks much better, will rename, thanks

jirka

> 
> > +                               val++;
> > +                       fmt = "idx 2/3: uretprobe";
> > +               }
> > +
> > +               ASSERT_EQ(skel->bss->uprobe_result[idx], val, fmt);
> > +               skel->bss->uprobe_result[idx] = 0;
> > +       }
> > +
> > +cleanup:
> > +       for (idx = 0; idx < 4; idx++)
> > +               uprobe_detach(skel, idx);
> > +}
> > +
> 
> [...]