* [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 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
* 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
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.