[v5,bpf-next,3/4] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

Commit Message

Dave Marchevsky Oct. 20, 2022, 4:07 p.m. UTC

Add a test_ringbuf_map_key test prog, borrowing heavily from extant
test_ringbuf.c. The program tries to use the result of
bpf_ringbuf_reserve as map_key, which was not possible before previouis
commits in this series. The test runner added to prog_tests/ringbuf.c
verifies that the program loads and does basic sanity checks to confirm
that it runs as expected.

Also, refactor test_ringbuf such that runners for existing test_ringbuf
and newly-added test_ringbuf_map_key are subtests of 'ringbuf' top-level
test.

Signed-off-by: Dave Marchevsky <[e-mail]>
Acked-by: Yonghong Song <[e-mail]>
---
v4->v5: lore.kernel.org/bpf/[e-mail]

* Fix some nits (Andrii)
  * migrating prog from fentry -> ksyscall wasn't done as lskel doesn't
    support the latter. Talked to Andrii about it offlist, he's fine with it.

v3->v4: lore.kernel.org/bpf/[e-mail]

* Fix some nits (Yonghong)
  * make subtest runner functions static
  * don't goto cleanup if -EDONE check fails
  * add 'workaround' to comment in test to ease future grepping
* Add Yonghong ack

v2->v3: lore.kernel.org/bpf/[e-mail]

* Test that ring_buffer__poll returns -EDONE (Alexei)

v1->v2: lore.kernel.org/bpf/[e-mail]

* Actually run the program instead of just loading (Yonghong)
* Add a bpf_map_update_elem call to the test (Yonghong)
* Refactor runner such that existing test and newly-added test are
  subtests of 'ringbuf' top-level test (Yonghong)
* Remove unused globals in test prog (Yonghong)

 tools/testing/selftests/bpf/Makefile          |  8 ++-
 .../selftests/bpf/prog_tests/ringbuf.c        | 66 ++++++++++++++++-
 .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
 3 files changed, 140 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c

Comments

Andrii Nakryiko Oct. 21, 2022, 11:04 p.m. UTC | #1

On Thu, Oct 20, 2022 at 9:07 AM Dave Marchevsky <[e-mail]> wrote:
>
> Add a test_ringbuf_map_key test prog, borrowing heavily from extant
> test_ringbuf.c. The program tries to use the result of
> bpf_ringbuf_reserve as map_key, which was not possible before previouis
> commits in this series. The test runner added to prog_tests/ringbuf.c
> verifies that the program loads and does basic sanity checks to confirm
> that it runs as expected.
>
> Also, refactor test_ringbuf such that runners for existing test_ringbuf
> and newly-added test_ringbuf_map_key are subtests of 'ringbuf' top-level
> test.
>
> Signed-off-by: Dave Marchevsky <[e-mail]>
> Acked-by: Yonghong Song <[e-mail]>
> ---

LGTM.

Acked-by: Andrii Nakryiko <[e-mail]>

> v4->v5: lore.kernel.org/bpf/[e-mail]
>
> * Fix some nits (Andrii)
>   * migrating prog from fentry -> ksyscall wasn't done as lskel doesn't
>     support the latter. Talked to Andrii about it offlist, he's fine with it.
>
> v3->v4: lore.kernel.org/bpf/[e-mail]
>
> * Fix some nits (Yonghong)
>   * make subtest runner functions static
>   * don't goto cleanup if -EDONE check fails
>   * add 'workaround' to comment in test to ease future grepping
> * Add Yonghong ack
>
> v2->v3: lore.kernel.org/bpf/[e-mail]
>
> * Test that ring_buffer__poll returns -EDONE (Alexei)
>
> v1->v2: lore.kernel.org/bpf/[e-mail]
>
> * Actually run the program instead of just loading (Yonghong)
> * Add a bpf_map_update_elem call to the test (Yonghong)
> * Refactor runner such that existing test and newly-added test are
>   subtests of 'ringbuf' top-level test (Yonghong)
> * Remove unused globals in test prog (Yonghong)
>
>  tools/testing/selftests/bpf/Makefile          |  8 ++-
>  .../selftests/bpf/prog_tests/ringbuf.c        | 66 ++++++++++++++++-
>  .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
>  3 files changed, 140 insertions(+), 4 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e6cf21fad69f..79edef1dbda4 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -359,9 +359,11 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h              \
>                 test_subskeleton.skel.h test_subskeleton_lib.skel.h     \
>                 test_usdt.skel.h

[...]

> diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c b/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> new file mode 100644
> index 000000000000..2760bf60d05a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct sample {
> +       int pid;
> +       int seq;
> +       long value;
> +       char comm[16];
> +};
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_RINGBUF);

btw, libbpf is smart enough now to auto-fix ringbuf size, so you could
have used __uint(max_entries, 4096) and that would work even on
architectures that have 64KB pages. Just FYI.

> +} ringbuf SEC(".maps");
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_HASH);
> +       __uint(max_entries, 1000);
> +       __type(key, struct sample);
> +       __type(value, int);
> +} hash_map SEC(".maps");
> +
> +/* inputs */
> +int pid = 0;
> +
> +/* inner state */
> +long seq = 0;
> +
> +SEC("fentry/" SYS_PREFIX "sys_getpgid")

it's fine as is, my suggestion to use ksyscall was to 1) avoid using
BPF trampoline (and so have these tests work on s390x) and 2) no have
to use ugly SYS_PREFIX. SEC("kprobe/" SYS_PREFIX "sys_getpgid") would
solve 1), which is more important in practical terms. 2) is a wishlist
:)

I'm not insisting or asking to change this, just pointing out the
rationale for ksyscall suggestion in the first place.


> +int test_ringbuf_mem_map_key(void *ctx)
> +{
> +       int cur_pid = bpf_get_current_pid_tgid() >> 32;
> +       struct sample *sample, sample_copy;
> +       int *lookup_val;
> +
> +       if (cur_pid != pid)
> +               return 0;
> +
> +       sample = bpf_ringbuf_reserve(&ringbuf, sizeof(*sample), 0);
> +       if (!sample)
> +               return 0;
> +
> +       sample->pid = pid;
> +       bpf_get_current_comm(sample->comm, sizeof(sample->comm));
> +       sample->seq = ++seq;
> +       sample->value = 42;
> +
> +       /* test using 'sample' (PTR_TO_MEM | MEM_ALLOC) as map key arg
> +        */
> +       lookup_val = (int *)bpf_map_lookup_elem(&hash_map, sample);
> +
> +       /* workaround - memcpy is necessary so that verifier doesn't
> +        * complain with:
> +        *   verifier internal error: more than one arg with ref_obj_id R3
> +        * when trying to do bpf_map_update_elem(&hash_map, sample, &sample->seq, BPF_ANY);
> +        *
> +        * Since bpf_map_lookup_elem above uses 'sample' as key, test using
> +        * sample field as value below
> +        */
> +       __builtin_memcpy(&sample_copy, sample, sizeof(struct sample));
> +       bpf_map_update_elem(&hash_map, &sample_copy, &sample->seq, BPF_ANY);
> +
> +       bpf_ringbuf_submit(sample, 0);
> +       return 0;
> +}
> --
> 2.30.2
>

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index e6cf21fad69f..79edef1dbda4 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -359,9 +359,11 @@  LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h		\
 		test_subskeleton.skel.h test_subskeleton_lib.skel.h	\
 		test_usdt.skel.h
 
-LSKELS := fentry_test.c fexit_test.c fexit_sleep.c \
-	test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
-	map_ptr_kern.c core_kern.c core_kern_overflow.c
+LSKELS := fentry_test.c fexit_test.c fexit_sleep.c atomics.c 		\
+	trace_printk.c trace_vprintk.c map_ptr_kern.c 			\
+	core_kern.c core_kern_overflow.c test_ringbuf.c			\
+	test_ringbuf_map_key.c
+
 # Generate both light skeleton and libbpf skeleton for these
 LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test.c \
 	kfunc_call_test_subprog.c
diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
index 9a80fe8a6427..ac104dc652e3 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
@@ -13,6 +13,7 @@ 
 #include <linux/perf_event.h>
 #include <linux/ring_buffer.h>
 #include "test_ringbuf.lskel.h"
+#include "test_ringbuf_map_key.lskel.h"
 
 #define EDONE 7777
 
@@ -58,6 +59,7 @@  static int process_sample(void *ctx, void *data, size_t len)
 	}
 }
 
+static struct test_ringbuf_map_key_lskel *skel_map_key;
 static struct test_ringbuf_lskel *skel;
 static struct ring_buffer *ringbuf;
 
@@ -81,7 +83,7 @@  static void *poll_thread(void *input)
 	return (void *)(long)ring_buffer__poll(ringbuf, timeout);
 }
 
-void test_ringbuf(void)
+static void ringbuf_subtest(void)
 {
 	const size_t rec_sz = BPF_RINGBUF_HDR_SZ + sizeof(struct sample);
 	pthread_t thread;
@@ -297,3 +299,65 @@  void test_ringbuf(void)
 	ring_buffer__free(ringbuf);
 	test_ringbuf_lskel__destroy(skel);
 }
+
+static int process_map_key_sample(void *ctx, void *data, size_t len)
+{
+	struct sample *s;
+	int err, val;
+
+	s = data;
+	switch (s->seq) {
+	case 1:
+		ASSERT_EQ(s->value, 42, "sample_value");
+		err = bpf_map_lookup_elem(skel_map_key->maps.hash_map.map_fd,
+					  s, &val);
+		ASSERT_OK(err, "hash_map bpf_map_lookup_elem");
+		ASSERT_EQ(val, 1, "hash_map val");
+		return -EDONE;
+	default:
+		return 0;
+	}
+}
+
+static void ringbuf_map_key_subtest(void)
+{
+	int err;
+
+	skel_map_key = test_ringbuf_map_key_lskel__open();
+	if (!ASSERT_OK_PTR(skel_map_key, "test_ringbuf_map_key_lskel__open"))
+		return;
+
+	skel_map_key->maps.ringbuf.max_entries = getpagesize();
+	skel_map_key->bss->pid = getpid();
+
+	err = test_ringbuf_map_key_lskel__load(skel_map_key);
+	if (!ASSERT_OK(err, "test_ringbuf_map_key_lskel__load"))
+		goto cleanup;
+
+	ringbuf = ring_buffer__new(skel_map_key->maps.ringbuf.map_fd,
+				   process_map_key_sample, NULL, NULL);
+	if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
+		goto cleanup;
+
+	err = test_ringbuf_map_key_lskel__attach(skel_map_key);
+	if (!ASSERT_OK(err, "test_ringbuf_map_key_lskel__attach"))
+		goto cleanup_ringbuf;
+
+	syscall(__NR_getpgid);
+	ASSERT_EQ(skel_map_key->bss->seq, 1, "skel_map_key->bss->seq");
+	err = ring_buffer__poll(ringbuf, -1);
+	ASSERT_EQ(err, -EDONE, "ring_buffer__poll");
+
+cleanup_ringbuf:
+	ring_buffer__free(ringbuf);
+cleanup:
+	test_ringbuf_map_key_lskel__destroy(skel_map_key);
+}
+
+void test_ringbuf(void)
+{
+	if (test__start_subtest("ringbuf"))
+		ringbuf_subtest();
+	if (test__start_subtest("ringbuf_map_key"))
+		ringbuf_map_key_subtest();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c b/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
new file mode 100644
index 000000000000..2760bf60d05a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
@@ -0,0 +1,70 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct sample {
+	int pid;
+	int seq;
+	long value;
+	char comm[16];
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_RINGBUF);
+} ringbuf SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(max_entries, 1000);
+	__type(key, struct sample);
+	__type(value, int);
+} hash_map SEC(".maps");
+
+/* inputs */
+int pid = 0;
+
+/* inner state */
+long seq = 0;
+
+SEC("fentry/" SYS_PREFIX "sys_getpgid")
+int test_ringbuf_mem_map_key(void *ctx)
+{
+	int cur_pid = bpf_get_current_pid_tgid() >> 32;
+	struct sample *sample, sample_copy;
+	int *lookup_val;
+
+	if (cur_pid != pid)
+		return 0;
+
+	sample = bpf_ringbuf_reserve(&ringbuf, sizeof(*sample), 0);
+	if (!sample)
+		return 0;
+
+	sample->pid = pid;
+	bpf_get_current_comm(sample->comm, sizeof(sample->comm));
+	sample->seq = ++seq;
+	sample->value = 42;
+
+	/* test using 'sample' (PTR_TO_MEM | MEM_ALLOC) as map key arg
+	 */
+	lookup_val = (int *)bpf_map_lookup_elem(&hash_map, sample);
+
+	/* workaround - memcpy is necessary so that verifier doesn't
+	 * complain with:
+	 *   verifier internal error: more than one arg with ref_obj_id R3
+	 * when trying to do bpf_map_update_elem(&hash_map, sample, &sample->seq, BPF_ANY);
+	 *
+	 * Since bpf_map_lookup_elem above uses 'sample' as key, test using
+	 * sample field as value below
+	 */
+	__builtin_memcpy(&sample_copy, sample, sizeof(struct sample));
+	bpf_map_update_elem(&hash_map, &sample_copy, &sample->seq, BPF_ANY);
+
+	bpf_ringbuf_submit(sample, 0);
+	return 0;
+}