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
> * but with different ref_ctr_offset which is not allowed and results in fail.
> */
> offsets[0] = tmp_offsets[0]; /* uprobe_multi_func_1 */
> @@ -722,6 +723,212 @@ static void test_link_api(void)
> __test_link_api(child);
> }
>
> +static struct bpf_program *
> +get_program(struct uprobe_multi_consumers *skel, int prog)
> +{
> + switch (prog) {
> + case 0:
> + return skel->progs.uprobe_0;
> + case 1:
> + return skel->progs.uprobe_1;
> + case 2:
> + return skel->progs.uprobe_2;
> + case 3:
> + return skel->progs.uprobe_3;
> + default:
> + ASSERT_FAIL("get_program");
> + return NULL;
> + }
> +}
> +
> +static struct bpf_link **
> +get_link(struct uprobe_multi_consumers *skel, int link)
> +{
> + switch (link) {
> + case 0:
> + return &skel->links.uprobe_0;
> + case 1:
> + return &skel->links.uprobe_1;
> + case 2:
> + return &skel->links.uprobe_2;
> + case 3:
> + return &skel->links.uprobe_3;
> + default:
> + ASSERT_FAIL("get_link");
> + return NULL;
> + }
> +}
> +
> +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?
> + "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 :)
> + unsigned long before, unsigned long after)
> +{
> + int idx;
> +
> + /* detach uprobe for each unset programs in 'before' state ... */
> + for (idx = 0; idx < 4; idx++) {
> + if (test_bit(idx, before) && !test_bit(idx, after))
> + uprobe_detach(skel, idx);
> + }
> +
> + /* ... and attach all new programs in 'after' state */
> + for (idx = 0; idx < 4; idx++) {
> + if (!test_bit(idx, before) && test_bit(idx, after)) {
> + if (!ASSERT_OK(uprobe_attach(skel, idx), "uprobe_attach_after"))
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> +static void session_consumer_test(struct uprobe_multi_consumers *skel,
> + unsigned long before, unsigned long after)
> +{
> + int err, idx;
> +
> + printf("session_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_session_consumer_test(skel, before, after);
> + if (!ASSERT_EQ(err, 0, "uprobe_session_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.
> + */
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".
> + 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);
> +}
> +
[...]
