| Message ID | 20221020160721.4030492-4-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8f4bc15b9ad73434643aadb19506e1547bedf7eb |
| Delegated to: | BPF |
| Headers | show |
| Series | [v5,bpf-next,1/4] bpf: Allow ringbuf memory to be used as map key | expand |
On Thu, Oct 20, 2022 at 9:07 AM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > Modify iter prog in existing bpf_iter_bpf_array_map.c, which currently > dumps arraymap key/val, to also do a write of (val, key) into a > newly-added hashmap. Confirm that the write succeeds as expected by > modifying the userspace runner program. > > Before a change added in an earlier commit - considering PTR_TO_BUF reg > a valid input to helpers which expect MAP_{KEY,VAL} - the verifier > would've rejected this prog change due to type mismatch. Since using > current iter's key/val to access a separate map is a reasonable usecase, > let's add support for it. > > Note that the test prog cannot directly write (val, key) into hashmap > via bpf_map_update_elem when both come from iter context because key is > marked MEM_RDONLY. This is due to bpf_map_update_elem - and other basic > map helpers - taking ARG_PTR_TO_MAP_{KEY,VALUE} w/o MEM_RDONLY type > flag. bpf_map_{lookup,update,delete}_elem don't modify their > input key/val so it should be possible to tag their args READONLY, but > due to the ubiquitous use of these helpers and verifier checks for > type == MAP_VALUE, such a change is nontrivial and seems better to > address in a followup series. Agree about addressing it separately, but I'm not sure what's non-trivial or dangerous? If I remember correctly, MEM_RDONLY on helper input arg just means that it accepts both read-only and read-write views. While the input argument doesn't have MEM_RDONLY we accept *only* read/write memory views. So basically adding MEM_RDONLY in BPF helper proto makes it more general and permissive in what can be passed into it. I think that's a good change, we added tons of MEM_RDONLY to helpers that were accepting PTR_TO_MEM already. But anyways, patch looks good: Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Also fixup some 'goto's in test runner's map checking loop. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > .../selftests/bpf/prog_tests/bpf_iter.c | 20 ++++++++++++------ > .../bpf/progs/bpf_iter_bpf_array_map.c | 21 ++++++++++++++++++- > 2 files changed, 34 insertions(+), 7 deletions(-) > [...]
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c index c39d40f4b268..6f8ed61fc4b4 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c @@ -941,10 +941,10 @@ static void test_bpf_array_map(void) { __u64 val, expected_val = 0, res_first_val, first_val = 0; DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts); - __u32 expected_key = 0, res_first_key; + __u32 key, expected_key = 0, res_first_key; + int err, i, map_fd, hash_fd, iter_fd; struct bpf_iter_bpf_array_map *skel; union bpf_iter_link_info linfo; - int err, i, map_fd, iter_fd; struct bpf_link *link; char buf[64] = {}; int len, start; @@ -1001,12 +1001,20 @@ static void test_bpf_array_map(void) if (!ASSERT_EQ(skel->bss->val_sum, expected_val, "val_sum")) goto close_iter; + hash_fd = bpf_map__fd(skel->maps.hashmap1); for (i = 0; i < bpf_map__max_entries(skel->maps.arraymap1); i++) { err = bpf_map_lookup_elem(map_fd, &i, &val); - if (!ASSERT_OK(err, "map_lookup")) - goto out; - if (!ASSERT_EQ(i, val, "invalid_val")) - goto out; + if (!ASSERT_OK(err, "map_lookup arraymap1")) + goto close_iter; + if (!ASSERT_EQ(i, val, "invalid_val arraymap1")) + goto close_iter; + + val = i + 4; + err = bpf_map_lookup_elem(hash_fd, &val, &key); + if (!ASSERT_OK(err, "map_lookup hashmap1")) + goto close_iter; + if (!ASSERT_EQ(key, val - 4, "invalid_val hashmap1")) + goto close_iter; } close_iter: diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_array_map.c b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_array_map.c index 6286023fd62b..c5969ca6f26b 100644 --- a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_array_map.c +++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_array_map.c @@ -19,13 +19,20 @@ struct { __type(value, __u64); } arraymap1 SEC(".maps"); +struct { + __uint(type, BPF_MAP_TYPE_HASH); + __uint(max_entries, 10); + __type(key, __u64); + __type(value, __u32); +} hashmap1 SEC(".maps"); + __u32 key_sum = 0; __u64 val_sum = 0; SEC("iter/bpf_map_elem") int dump_bpf_array_map(struct bpf_iter__bpf_map_elem *ctx) { - __u32 *key = ctx->key; + __u32 *hmap_val, *key = ctx->key; __u64 *val = ctx->value; if (key == (void *)0 || val == (void *)0) @@ -35,6 +42,18 @@ int dump_bpf_array_map(struct bpf_iter__bpf_map_elem *ctx) bpf_seq_write(ctx->meta->seq, val, sizeof(__u64)); key_sum += *key; val_sum += *val; + + /* workaround - It's necessary to do this convoluted (val, key) + * write into hashmap1, instead of simply doing + * bpf_map_update_elem(&hashmap1, val, key, BPF_ANY); + * because key has MEM_RDONLY flag and bpf_map_update elem expects + * types without this flag + */ + bpf_map_update_elem(&hashmap1, val, val, BPF_ANY); + hmap_val = bpf_map_lookup_elem(&hashmap1, val); + if (hmap_val) + *hmap_val = *key; + *val = *key; return 0; }
Modify iter prog in existing bpf_iter_bpf_array_map.c, which currently dumps arraymap key/val, to also do a write of (val, key) into a newly-added hashmap. Confirm that the write succeeds as expected by modifying the userspace runner program. Before a change added in an earlier commit - considering PTR_TO_BUF reg a valid input to helpers which expect MAP_{KEY,VAL} - the verifier would've rejected this prog change due to type mismatch. Since using current iter's key/val to access a separate map is a reasonable usecase, let's add support for it. Note that the test prog cannot directly write (val, key) into hashmap via bpf_map_update_elem when both come from iter context because key is marked MEM_RDONLY. This is due to bpf_map_update_elem - and other basic map helpers - taking ARG_PTR_TO_MAP_{KEY,VALUE} w/o MEM_RDONLY type flag. bpf_map_{lookup,update,delete}_elem don't modify their input key/val so it should be possible to tag their args READONLY, but due to the ubiquitous use of these helpers and verifier checks for type == MAP_VALUE, such a change is nontrivial and seems better to address in a followup series. Also fixup some 'goto's in test runner's map checking loop. Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- .../selftests/bpf/prog_tests/bpf_iter.c | 20 ++++++++++++------ .../bpf/progs/bpf_iter_bpf_array_map.c | 21 ++++++++++++++++++- 2 files changed, 34 insertions(+), 7 deletions(-)