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";