Messages in this thread | | | Date | Thu, 1 Jun 2023 12:18:15 -0700 | Subject | Re: [PATCH bpf] bpf: Fix elem_size not being set for inner maps | From | Martin KaFai Lau <> |
| |
On 5/31/23 5:08 PM, Rhys Rustad-Elliott wrote: > Commit d937bc3449fa ("bpf: make uniform use of array->elem_size > everywhere in arraymap.c") changed array_map_gen_lookup to use > array->elem_size instead of round_up(map->value_size, 8) as the element > size when generating code to access a value in an array map. > > array->elem_size, however, is not set by bpf_map_meta_alloc when > initializing an BPF_MAP_TYPE_ARRAY_OF_MAPS or BPF_MAP_TYPE_HASH_OF_MAPS. > This results in array_map_gen_lookup incorrectly outputting code that > always accesses index 0 in the array (as the index will be calculated > via a multiplication with the element size, which is incorrectly set to > 0). > > Set elem_size on the bpf_array object when allocating an array or hash > of maps and add a selftest that accesses an inner map at a nonzero index > to prevent regressions. > > Fixes: d937bc3449fa ("bpf: make uniform use of array->elem_size everywhere in arraymap.c") > Signed-off-by: Rhys Rustad-Elliott <me@rhysre.net> > --- > kernel/bpf/map_in_map.c | 8 +++- > .../map_in_map_inner_array_lookup.c | 33 ++++++++++++++ > .../test_map_in_map_inner_array_lookup.c | 45 +++++++++++++++++++ > 3 files changed, 84 insertions(+), 2 deletions(-) > create mode 100644 tools/testing/selftests/bpf/prog_tests/map_in_map_inner_array_lookup.c > create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_inner_array_lookup.c > > diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c > index 2c5c64c2a53b..8d65b12e0834 100644 > --- a/kernel/bpf/map_in_map.c > +++ b/kernel/bpf/map_in_map.c > @@ -69,9 +69,13 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd) > /* Misc members not needed in bpf_map_meta_equal() check. */ > inner_map_meta->ops = inner_map->ops; > if (inner_map->ops == &array_map_ops) { > + struct bpf_array *inner_array_meta = > + container_of(inner_map_meta, struct bpf_array, map); > + struct bpf_array *inner_array = container_of(inner_map, struct bpf_array, map); > + > + inner_array_meta->index_mask = inner_array->index_mask; > + inner_array_meta->elem_size = round_up(inner_map->value_size, 8);
How about directly use inner_array->elem_size instead of "round_up(inner_map->value_size, 8)"?
> inner_map_meta->bypass_spec_v1 = inner_map->bypass_spec_v1; > - container_of(inner_map_meta, struct bpf_array, map)->index_mask = > - container_of(inner_map, struct bpf_array, map)->index_mask; > } > > fdput(f); > diff --git a/tools/testing/selftests/bpf/prog_tests/map_in_map_inner_array_lookup.c b/tools/testing/selftests/bpf/prog_tests/map_in_map_inner_array_lookup.c > new file mode 100644 > index 000000000000..264d4788e5fd > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/map_in_map_inner_array_lookup.c
Separate the selftests into another patch.
> @@ -0,0 +1,33 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include <test_progs.h> > + > +#include "test_map_in_map_inner_array_lookup.skel.h" > + > +static int duration;
Use the ASSERT_* macro instead of CHECK, then no need for "static int duration;".
> + > +void test_map_in_map_inner_array_lookup(void)
nit. A shorter name? may be test_inner_array_lookup().
> +{ > + int map1_fd, err; > + int key = 3; > + int val = 1; > + struct test_map_in_map_inner_array_lookup *skel; > + > + skel = test_map_in_map_inner_array_lookup__open_and_load(); > + if (CHECK(!skel, "skel_open", "failed to open&load skeleton\n")) > + return; > + > + err = test_map_in_map_inner_array_lookup__attach(skel); > + if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err)) > + goto cleanup; > + > + map1_fd = bpf_map__fd(skel->maps.inner_map1); > + bpf_map_update_elem(map1_fd, &key, &val, 0); > + usleep(1);
Why usleep is needed?
> + /* Probe should have set the element at index 3 to 2 */ > + bpf_map_lookup_elem(map1_fd, &key, &val); > + CHECK(val != 2, "inner1", "got %d != exp %d\n", val, 2); > + > +cleanup: > + test_map_in_map_inner_array_lookup__destroy(skel); > +} > diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map_inner_array_lookup.c b/tools/testing/selftests/bpf/progs/test_map_in_map_inner_array_lookup.c > new file mode 100644 > index 000000000000..c2c8f2fa451d > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_map_in_map_inner_array_lookup.c
nit. A shorter name also, inner_array_lookup.c?
> @@ -0,0 +1,45 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > + > +struct inner_map { > + __uint(type, BPF_MAP_TYPE_ARRAY); > + __uint(max_entries, 5); > + __type(key, int); > + __type(value, int); > +} inner_map1 SEC(".maps"); > + > +struct outer_map { > + __uint(type, BPF_MAP_TYPE_HASH_OF_MAPS); > + __uint(max_entries, 3); > + __type(key, int); > + __array(values, struct inner_map); > +} outer_map1 SEC(".maps") = { > + .values = { > + [2] = &inner_map1, > + }, > +}; > + > +SEC("raw_tp/sys_enter") > +int handle__sys_enter(void *ctx) > +{ > + int outer_key = 2, inner_key = 3; > + int *val; > + void *map; > + > + map = bpf_map_lookup_elem(&outer_map1, &outer_key); > + if (!map) > + return 1; > + > + val = bpf_map_lookup_elem(map, &inner_key); > + if (!val) > + return 1; > + > + if (*val == 1) > + *val = 2; > + > + return 0; > +} > + > +char _license[] SEC("license") = "GPL";
| |