| Message ID | 20220914123600.927632-2-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2,bpf-next,1/2] bpf: Allow ringbuf memory to be used as map key | expand |
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.
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; > +}
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.
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.
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.
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.
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; +}
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