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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > .../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);
> > +}
> > +
>
> [...]
