[PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key

[PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key

[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

[PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[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

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[Alexei Starovoitov]

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.

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[Yonghong Song]

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

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[Kumar Kartikeya Dwivedi]

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.

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[Yonghong Song]

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.

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[Dave Marchevsky]

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.

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[Yonghong Song]

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.

Re: [PATCH v2 bpf-next 1/2] bpf: Allow ringbuf memory to be used as map key

[Yonghong Song]

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>