All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key
@ 2022-09-14 12:35 Dave Marchevsky
  2022-09-14 12:36 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops Dave Marchevsky
  2022-09-14 17:21 ` [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key Yonghong Song
  0 siblings, 2 replies; 9+ messages in thread
From: Dave Marchevsky @ 2022-09-14 12:35 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Kernel Team, Kumar Kartikeya Dwivedi, Yonghong Song,
	Dave Marchevsky

This patch adds support for the following pattern:

  struct some_data *data = bpf_ringbuf_reserve(&ringbuf, sizeof(struct some_data, 0));
  if (!data)
    return;
  bpf_map_lookup_elem(&another_map, &data->some_field);
  bpf_ringbuf_submit(data);

Currently the verifier does not consider bpf_ringbuf_reserve's
PTR_TO_MEM | MEM_ALLOC ret type a valid key input to bpf_map_lookup_elem.
Since PTR_TO_MEM is by definition a valid region of memory, it is safe
to use it as a key for lookups.

Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com

  * Move test changes into separate patch - patch 2 in this series.
    (Kumar, Yonghong). That patch's changelog enumerates specific
    changes from v1
  * Remove PTR_TO_MEM addition from this patch - patch 1 (Yonghong)
    * I don't have a usecase for PTR_TO_MEM w/o MEM_ALLOC
  * Add "if (!data)" error check to example pattern in this patch
    (Yonghong)
  * Remove patch 2 from v1's series, which removed map_key_value_types
    as it was more-or-less duplicate of mem_types
    * Now that PTR_TO_MEM isn't added here, more differences between
      map_key_value_types and mem_types, and no usecase for PTR_TO_BUF,
      so drop for now.

 kernel/bpf/verifier.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c259d734f863..f6e029780698 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5626,6 +5626,7 @@ static const struct bpf_reg_types map_key_value_types = {
 		PTR_TO_PACKET_META,
 		PTR_TO_MAP_KEY,
 		PTR_TO_MAP_VALUE,
+		PTR_TO_MEM | MEM_ALLOC,
 	},
 };
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops
  2022-09-14 12:35 [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key Dave Marchevsky
@ 2022-09-14 12:36 ` Dave Marchevsky
  2022-09-15 10:24   ` Alexei Starovoitov
  2022-09-19 22:53   ` Yonghong Song
  2022-09-14 17:21 ` [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key Yonghong Song
  1 sibling, 2 replies; 9+ messages in thread
From: Dave Marchevsky @ 2022-09-14 12:36 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Kernel Team, Kumar Kartikeya Dwivedi, Yonghong Song,
	Dave Marchevsky

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 <davemarchevsky@fb.com>
---
v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com

* 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        | 63 ++++++++++++++++-
 .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
 3 files changed, 137 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 6cd327f1f216..231d9c1364c9 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -351,9 +351,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..e0f8db69cb77 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)
+void ringbuf_subtest(void)
 {
 	const size_t rec_sz = BPF_RINGBUF_HDR_SZ + sizeof(struct sample);
 	pthread_t thread;
@@ -297,3 +299,62 @@ 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;
+	}
+}
+
+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);
+
+	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");
+	ring_buffer__poll(ringbuf, -1);
+
+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..495f85c6e120
--- /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);
+	__uint(max_entries, 4096);
+} 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);
+
+	/* 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key
  2022-09-14 12:35 [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key Dave Marchevsky
  2022-09-14 12:36 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops Dave Marchevsky
@ 2022-09-14 17:21 ` Yonghong Song
  1 sibling, 0 replies; 9+ messages in thread
From: Yonghong Song @ 2022-09-14 17:21 UTC (permalink / raw)
  To: Dave Marchevsky, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Kernel Team, Kumar Kartikeya Dwivedi



On 9/14/22 1:35 PM, Dave Marchevsky wrote:
> This patch adds support for the following pattern:
> 
>    struct some_data *data = bpf_ringbuf_reserve(&ringbuf, sizeof(struct some_data, 0));
>    if (!data)
>      return;
>    bpf_map_lookup_elem(&another_map, &data->some_field);
>    bpf_ringbuf_submit(data);
> 
> Currently the verifier does not consider bpf_ringbuf_reserve's
> PTR_TO_MEM | MEM_ALLOC ret type a valid key input to bpf_map_lookup_elem.
> Since PTR_TO_MEM is by definition a valid region of memory, it is safe
> to use it as a key for lookups.
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>

Acked-by: Yonghong Song <yhs@fb.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops
  2022-09-14 12:36 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops Dave Marchevsky
@ 2022-09-15 10:24   ` Alexei Starovoitov
  2022-09-19 22:53   ` Yonghong Song
  1 sibling, 0 replies; 9+ messages in thread
From: Alexei Starovoitov @ 2022-09-15 10:24 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Kernel Team, Kumar Kartikeya Dwivedi, Yonghong Song

On Wed, Sep 14, 2022 at 1:36 PM Dave Marchevsky <davemarchevsky@fb.com> 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 <davemarchevsky@fb.com>
> ---
> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com
>
> * 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        | 63 ++++++++++++++++-
>  .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
>  3 files changed, 137 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 6cd327f1f216..231d9c1364c9 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -351,9 +351,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..e0f8db69cb77 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)
> +void ringbuf_subtest(void)
>  {
>         const size_t rec_sz = BPF_RINGBUF_HDR_SZ + sizeof(struct sample);
>         pthread_t thread;
> @@ -297,3 +299,62 @@ 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;
> +       }
> +}
> +
> +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);
> +
> +       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");
> +       ring_buffer__poll(ringbuf, -1);

Why is there no err == EDONE check here?
Without the check the prog could have skipped
ringbuf_submit and process_map_key_sample() above would not
be called.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops
  2022-09-14 12:36 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops Dave Marchevsky
  2022-09-15 10:24   ` Alexei Starovoitov
@ 2022-09-19 22:53   ` Yonghong Song
  2022-09-19 23:22     ` Kumar Kartikeya Dwivedi
  1 sibling, 1 reply; 9+ messages in thread
From: Yonghong Song @ 2022-09-19 22:53 UTC (permalink / raw)
  To: Dave Marchevsky, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Kernel Team, Kumar Kartikeya Dwivedi



On 9/14/22 5:36 AM, Dave Marchevsky 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 <davemarchevsky@fb.com>
> ---
> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com
> 
> * 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        | 63 ++++++++++++++++-
>   .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
>   3 files changed, 137 insertions(+), 4 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> 
[...]
> 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..495f85c6e120
> --- /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);
> +	__uint(max_entries, 4096);
> +} 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);
> +
> +	/* 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
> +	 */

If I understand correctly, the above error is due to the following 
verifier code:

         if (reg->ref_obj_id) {
                 if (meta->ref_obj_id) {
                         verbose(env, "verifier internal error: more 
than one arg with ref_obj_id R%d %u %u\n",
                                 regno, reg->ref_obj_id,
                                 meta->ref_obj_id);
                         return -EFAULT;
                 }
                 meta->ref_obj_id = reg->ref_obj_id;
         }

So this is an internal error. So normally this should not happen.
Could you investigate and fix the issue?

> +	__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;
> +}

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops
  2022-09-19 22:53   ` Yonghong Song
@ 2022-09-19 23:22     ` Kumar Kartikeya Dwivedi
  2022-09-20  5:50       ` Yonghong Song
  0 siblings, 1 reply; 9+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2022-09-19 23:22 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Dave Marchevsky, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team

On Tue, 20 Sept 2022 at 00:53, Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 9/14/22 5:36 AM, Dave Marchevsky 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 <davemarchevsky@fb.com>
> > ---
> > v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com
> >
> > * 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        | 63 ++++++++++++++++-
> >   .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
> >   3 files changed, 137 insertions(+), 4 deletions(-)
> >   create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> >
> [...]
> > 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..495f85c6e120
> > --- /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);
> > +     __uint(max_entries, 4096);
> > +} 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);
> > +
> > +     /* 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
> > +      */
>
> If I understand correctly, the above error is due to the following
> verifier code:
>
>          if (reg->ref_obj_id) {
>                  if (meta->ref_obj_id) {
>                          verbose(env, "verifier internal error: more
> than one arg with ref_obj_id R%d %u %u\n",
>                                  regno, reg->ref_obj_id,
>                                  meta->ref_obj_id);
>                          return -EFAULT;
>                  }
>                  meta->ref_obj_id = reg->ref_obj_id;
>          }
>
> So this is an internal error. So normally this should not happen.
> Could you investigate and fix the issue?
>

Technically it's not an "internal" error, it's totally possible to
pass two referenced registers from a program (which the verifier
rejects). So a bad log message I guess.

We probably need to update the verifier to properly recognize the
ref_obj_id for certain functions. For release arguments we already
have meta.release_regno/OBJ_RELEASE for. It can already find the
ref_obj_id from release_regno instead of meta.ref_obj_id.

For dynptr_ref or ptr_cast, simply store meta.ref_obj_id by capturing
the regno and then setting it before r1-r5 is cleared.
Since that is passed to r0 it will be done later after clearing of
caller saved regs.
ptr_cast and dynptr_ref functions are already exclusive (due to
helper_multiple_ref_obj_use) so they can share the same regno field in
meta.

Then remove this check on seeing more than one reg->ref_obj_id, so it
isn't a problem to allow more than one refcounted registers for all
other arguments, as long as we correctly remember the ones for the
cases we care about.

But it can probably be a separate change from this.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops
  2022-09-19 23:22     ` Kumar Kartikeya Dwivedi
@ 2022-09-20  5:50       ` Yonghong Song
  2022-09-22 14:27         ` Dave Marchevsky
  0 siblings, 1 reply; 9+ messages in thread
From: Yonghong Song @ 2022-09-20  5:50 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: Dave Marchevsky, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team



On 9/19/22 4:22 PM, Kumar Kartikeya Dwivedi wrote:
> On Tue, 20 Sept 2022 at 00:53, Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 9/14/22 5:36 AM, Dave Marchevsky 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 <davemarchevsky@fb.com>
>>> ---
>>> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com
>>>
>>> * 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        | 63 ++++++++++++++++-
>>>    .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
>>>    3 files changed, 137 insertions(+), 4 deletions(-)
>>>    create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
>>>
>> [...]
>>> 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..495f85c6e120
>>> --- /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);
>>> +     __uint(max_entries, 4096);
>>> +} 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);
>>> +
>>> +     /* 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
>>> +      */
>>
>> If I understand correctly, the above error is due to the following
>> verifier code:
>>
>>           if (reg->ref_obj_id) {
>>                   if (meta->ref_obj_id) {
>>                           verbose(env, "verifier internal error: more
>> than one arg with ref_obj_id R%d %u %u\n",
>>                                   regno, reg->ref_obj_id,
>>                                   meta->ref_obj_id);
>>                           return -EFAULT;
>>                   }
>>                   meta->ref_obj_id = reg->ref_obj_id;
>>           }
>>
>> So this is an internal error. So normally this should not happen.
>> Could you investigate and fix the issue?
>>
> 
> Technically it's not an "internal" error, it's totally possible to
> pass two referenced registers from a program (which the verifier
> rejects). So a bad log message I guess.
> 
> We probably need to update the verifier to properly recognize the
> ref_obj_id for certain functions. For release arguments we already
> have meta.release_regno/OBJ_RELEASE for. It can already find the
> ref_obj_id from release_regno instead of meta.ref_obj_id.
> 
> For dynptr_ref or ptr_cast, simply store meta.ref_obj_id by capturing
> the regno and then setting it before r1-r5 is cleared.
> Since that is passed to r0 it will be done later after clearing of
> caller saved regs.
> ptr_cast and dynptr_ref functions are already exclusive (due to
> helper_multiple_ref_obj_use) so they can share the same regno field in
> meta.
> 
> Then remove this check on seeing more than one reg->ref_obj_id, so it
> isn't a problem to allow more than one refcounted registers for all
> other arguments, as long as we correctly remember the ones for the
> cases we care about.

Thanks for the explanation!

> 
> But it can probably be a separate change from this.

if the use case this patch set tried to address is using
bpf_map_update_elem(), we should fix the double
ref_obj_id in the current patch set. If only
bpf_map_lookup_elem() is needed. Then we can delay
the verifier change for the followup patch.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops
  2022-09-20  5:50       ` Yonghong Song
@ 2022-09-22 14:27         ` Dave Marchevsky
  2022-09-22 16:59           ` Yonghong Song
  0 siblings, 1 reply; 9+ messages in thread
From: Dave Marchevsky @ 2022-09-22 14:27 UTC (permalink / raw)
  To: Yonghong Song, Kumar Kartikeya Dwivedi
  Cc: Dave Marchevsky, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team

On 9/20/22 1:50 AM, Yonghong Song wrote:
> 
> 
> On 9/19/22 4:22 PM, Kumar Kartikeya Dwivedi wrote:
>> On Tue, 20 Sept 2022 at 00:53, Yonghong Song <yhs@fb.com> wrote:
>>>
>>>
>>>
>>> On 9/14/22 5:36 AM, Dave Marchevsky 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 <davemarchevsky@fb.com>
>>>> ---
>>>> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com
>>>>
>>>> * 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        | 63 ++++++++++++++++-
>>>>    .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
>>>>    3 files changed, 137 insertions(+), 4 deletions(-)
>>>>    create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
>>>>
>>> [...]
>>>> 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..495f85c6e120
>>>> --- /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);
>>>> +     __uint(max_entries, 4096);
>>>> +} 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);
>>>> +
>>>> +     /* 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
>>>> +      */
>>>
>>> If I understand correctly, the above error is due to the following
>>> verifier code:
>>>
>>>           if (reg->ref_obj_id) {
>>>                   if (meta->ref_obj_id) {
>>>                           verbose(env, "verifier internal error: more
>>> than one arg with ref_obj_id R%d %u %u\n",
>>>                                   regno, reg->ref_obj_id,
>>>                                   meta->ref_obj_id);
>>>                           return -EFAULT;
>>>                   }
>>>                   meta->ref_obj_id = reg->ref_obj_id;
>>>           }
>>>
>>> So this is an internal error. So normally this should not happen.
>>> Could you investigate and fix the issue?
>>>
>>
>> Technically it's not an "internal" error, it's totally possible to
>> pass two referenced registers from a program (which the verifier
>> rejects). So a bad log message I guess.
>>
>> We probably need to update the verifier to properly recognize the
>> ref_obj_id for certain functions. For release arguments we already
>> have meta.release_regno/OBJ_RELEASE for. It can already find the
>> ref_obj_id from release_regno instead of meta.ref_obj_id.
>>
>> For dynptr_ref or ptr_cast, simply store meta.ref_obj_id by capturing
>> the regno and then setting it before r1-r5 is cleared.
>> Since that is passed to r0 it will be done later after clearing of
>> caller saved regs.
>> ptr_cast and dynptr_ref functions are already exclusive (due to
>> helper_multiple_ref_obj_use) so they can share the same regno field in
>> meta.
>>
>> Then remove this check on seeing more than one reg->ref_obj_id, so it
>> isn't a problem to allow more than one refcounted registers for all
>> other arguments, as long as we correctly remember the ones for the
>> cases we care about.
> 
> Thanks for the explanation!
> 
>>
>> But it can probably be a separate change from this.
> 
> if the use case this patch set tried to address is using
> bpf_map_update_elem(), we should fix the double
> ref_obj_id in the current patch set. If only
> bpf_map_lookup_elem() is needed. Then we can delay
> the verifier change for the followup patch.
> 

The bpf_map_lookup_elem() usecase is the only one critical for me, so I've
submitted v3 without ref_obj_id fix. I agree that it should be fixed, but feels
orthogonal to this change, and is probably best addressed as a verifier-wide
fix affecting all functions as per Kumar's suggestion.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops
  2022-09-22 14:27         ` Dave Marchevsky
@ 2022-09-22 16:59           ` Yonghong Song
  0 siblings, 0 replies; 9+ messages in thread
From: Yonghong Song @ 2022-09-22 16:59 UTC (permalink / raw)
  To: Dave Marchevsky, Kumar Kartikeya Dwivedi
  Cc: Dave Marchevsky, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team



On 9/22/22 7:27 AM, Dave Marchevsky wrote:
> On 9/20/22 1:50 AM, Yonghong Song wrote:
>>
>>
>> On 9/19/22 4:22 PM, Kumar Kartikeya Dwivedi wrote:
>>> On Tue, 20 Sept 2022 at 00:53, Yonghong Song <yhs@fb.com> wrote:
>>>>
>>>>
>>>>
>>>> On 9/14/22 5:36 AM, Dave Marchevsky 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 <davemarchevsky@fb.com>
>>>>> ---
>>>>> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com
>>>>>
>>>>> * 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        | 63 ++++++++++++++++-
>>>>>     .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
>>>>>     3 files changed, 137 insertions(+), 4 deletions(-)
>>>>>     create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
>>>>>
>>>> [...]
>>>>> 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..495f85c6e120
>>>>> --- /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);
>>>>> +     __uint(max_entries, 4096);
>>>>> +} 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);
>>>>> +
>>>>> +     /* 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
>>>>> +      */
>>>>
>>>> If I understand correctly, the above error is due to the following
>>>> verifier code:
>>>>
>>>>            if (reg->ref_obj_id) {
>>>>                    if (meta->ref_obj_id) {
>>>>                            verbose(env, "verifier internal error: more
>>>> than one arg with ref_obj_id R%d %u %u\n",
>>>>                                    regno, reg->ref_obj_id,
>>>>                                    meta->ref_obj_id);
>>>>                            return -EFAULT;
>>>>                    }
>>>>                    meta->ref_obj_id = reg->ref_obj_id;
>>>>            }
>>>>
>>>> So this is an internal error. So normally this should not happen.
>>>> Could you investigate and fix the issue?
>>>>
>>>
>>> Technically it's not an "internal" error, it's totally possible to
>>> pass two referenced registers from a program (which the verifier
>>> rejects). So a bad log message I guess.
>>>
>>> We probably need to update the verifier to properly recognize the
>>> ref_obj_id for certain functions. For release arguments we already
>>> have meta.release_regno/OBJ_RELEASE for. It can already find the
>>> ref_obj_id from release_regno instead of meta.ref_obj_id.
>>>
>>> For dynptr_ref or ptr_cast, simply store meta.ref_obj_id by capturing
>>> the regno and then setting it before r1-r5 is cleared.
>>> Since that is passed to r0 it will be done later after clearing of
>>> caller saved regs.
>>> ptr_cast and dynptr_ref functions are already exclusive (due to
>>> helper_multiple_ref_obj_use) so they can share the same regno field in
>>> meta.
>>>
>>> Then remove this check on seeing more than one reg->ref_obj_id, so it
>>> isn't a problem to allow more than one refcounted registers for all
>>> other arguments, as long as we correctly remember the ones for the
>>> cases we care about.
>>
>> Thanks for the explanation!
>>
>>>
>>> But it can probably be a separate change from this.
>>
>> if the use case this patch set tried to address is using
>> bpf_map_update_elem(), we should fix the double
>> ref_obj_id in the current patch set. If only
>> bpf_map_lookup_elem() is needed. Then we can delay
>> the verifier change for the followup patch.
>>
> 
> The bpf_map_lookup_elem() usecase is the only one critical for me, so I've
> submitted v3 without ref_obj_id fix. I agree that it should be fixed, but feels
> orthogonal to this change, and is probably best addressed as a verifier-wide
> fix affecting all functions as per Kumar's suggestion.

Okay. This works for me. The ref_obj_id fix can be a followup.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-09-22 17:00 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-14 12:35 [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key Dave Marchevsky
2022-09-14 12:36 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops Dave Marchevsky
2022-09-15 10:24   ` Alexei Starovoitov
2022-09-19 22:53   ` Yonghong Song
2022-09-19 23:22     ` Kumar Kartikeya Dwivedi
2022-09-20  5:50       ` Yonghong Song
2022-09-22 14:27         ` Dave Marchevsky
2022-09-22 16:59           ` Yonghong Song
2022-09-14 17:21 ` [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key Yonghong Song

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.