On Tue, Sep 24, 2024 at 2:33 AM Ihor Solodrai <ihor.solodrai@xxxxx> wrote:
>
> On Monday, July 22nd, 2024 at 1:27 PM, Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> >
> >
> > Adding test that attaches/detaches multiple consumers on
> > single uprobe and verifies all were hit as expected.
> >
> > Signed-off-by: Jiri Olsa jolsa@xxxxxxxxxx
> >
> > ---
> > .../bpf/prog_tests/uprobe_multi_test.c | 213 ++++++++++++++++++
> > .../bpf/progs/uprobe_multi_consumers.c | 39 ++++
> > 2 files changed, 252 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_consumers.c
> >
> > 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 e6255d4df81d..27708110ea20 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"
> > @@ -731,6 +732,216 @@ static void test_link_api(void)
> > __test_link_api(child);
> > }
> >
>
> [...]
>
> > +
> > +static void consumer_test(struct uprobe_multi_consumers skel,
> > + unsigned long before, unsigned long after)
> > +{
> > + int err, idx;
> > +
> > + printf("consumer_test before %lu after %lu\n", before, after);
> > +
> > + / 'before' is each, we attach uprobe for every set idx */
> > + for (idx = 0; idx < 4; idx++) {
> > + if (test_bit(idx, before)) {
> > + if (!ASSERT_OK(uprobe_attach(skel, idx), "uprobe_attach_before"))
> > + goto cleanup;
> > + }
> > + }
> > +
> > + err = uprobe_consumer_test(skel, before, after);
> > + if (!ASSERT_EQ(err, 0, "uprobe_consumer_test"))
> > + goto cleanup;
> > +
> > + for (idx = 0; idx < 4; idx++) {
> > + const char fmt = "BUG";
> > + __u64 val = 0;
> > +
> > + if (idx < 2) {
> > + /
> > + * uprobe entry
> > + * +1 if define in 'before'
> > + /
> > + if (test_bit(idx, before))
> > + val++;
> > + fmt = "prog 0/1: uprobe";
> > + } 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.
> > + */
> > + unsigned long had_uretprobes = before & 0b1100; // is uretprobe installed
> > + unsigned long probe_preserved = before & after; // did uprobe go away
> > +
> > + if (had_uretprobes && probe_preserved && test_bit(idx, after))
> > + val++;
> > + fmt = "idx 2/3: uretprobe";
> > + }
>
> Jiri, Andrii,
>
> This test case started failing since upstream got merged into bpf-next,
> starting from commit https://git.kernel.org/bpf/bpf-next/c/440b65232829
>
> A snippet from the test log:
>
> consumer_test before 4 after 8
> uprobe_attach:PASS:bpf_program__attach_uprobe_multi 0 nsec
> consumer_test:PASS:uprobe_attach_before 0 nsec
> uprobe_attach:PASS:bpf_program__attach_uprobe_multi 0 nsec
> uprobe_consumer_test:PASS:uprobe_attach_after 0 nsec
> consumer_test:PASS:uprobe_consumer_test 0 nsec
> consumer_test:PASS:prog 0/1: uprobe 0 nsec
> consumer_test:PASS:prog 0/1: uprobe 0 nsec
> consumer_test:PASS:idx 2/3: uretprobe 0 nsec
> consumer_test:FAIL:idx 2/3: uretprobe unexpected idx 2/3: uretprobe: actual 1 != expected 0
> consumer_test before 4 after 9
> uprobe_attach:PASS:bpf_program__attach_uprobe_multi 0 nsec
> consumer_test:PASS:uprobe_attach_before 0 nsec
> uprobe_attach:PASS:bpf_program__attach_uprobe_multi 0 nsec
> uprobe_consumer_test:PASS:uprobe_attach_after 0 nsec
> uprobe_attach:PASS:bpf_program__attach_uprobe_multi 0 nsec
> uprobe_consumer_test:PASS:uprobe_attach_after 0 nsec
> consumer_test:PASS:uprobe_consumer_test 0 nsec
> consumer_test:PASS:prog 0/1: uprobe 0 nsec
> consumer_test:PASS:prog 0/1: uprobe 0 nsec
> consumer_test:PASS:idx 2/3: uretprobe 0 nsec
> consumer_test:FAIL:idx 2/3: uretprobe unexpected idx 2/3: uretprobe: actual 1 != expected 0
> consumer_test before 4 after 10
> uprobe_attach:PASS:bpf_program__attach_uprobe_multi 0 nsec
> consumer_test:PASS:uprobe_attach_before 0 nsec
> uprobe_attach:PASS:bpf_program__attach_uprobe_multi 0 nsec
> uprobe_consumer_test:PASS:uprobe_attach_after 0 nsec
> uprobe_attach:PASS:bpf_program__attach_uprobe_multi 0 nsec
> uprobe_consumer_test:PASS:uprobe_attach_after 0 nsec
> consumer_test:PASS:uprobe_consumer_test 0 nsec
> consumer_test:PASS:prog 0/1: uprobe 0 nsec
> consumer_test:PASS:prog 0/1: uprobe 0 nsec
> consumer_test:PASS:idx 2/3: uretprobe 0 nsec
> consumer_test:FAIL:idx 2/3: uretprobe unexpected idx 2/3: uretprobe: actual 1 != expected 0
>
>
> I couldn't figure out the reason as I have very shallow understanding
> of what's happening in the test.
>
> Jiri, could you please look into it?
>
> I excluded this test from BPF CI for now.
Thanks for the mitigation! I think this is due to my recent RCU and
refcounting changes to uprobes/uretprobes, which went through
tip/perf/core initially. And now that tip and bpf-next trees
converged, this condition:
> unsigned long probe_preserved = before & after; // did uprobe go away
is no longer correct, and uretprobe can be activated if there was
*any* uretprobe installed before.
So the test needs adjustment, but I don't think anything really broke.
I don't remember exactly (and given the conferencing schedule and
quite bad internet can't test quickly), but I think the condition
should now be:
unsigned long probe_preserved = after & 0x1100;
(though we might want to also rename the variable to be a bit more
meaningful now).
Anyways, I don't think this is critical and we can address this later.
But if anyone is willing to send a fix, I'd appreciate it, of course!
>
> Thank you!
>
> > +
> > + 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);
> > +}
>
> [...]
>
