On Fri, Jul 16, 2021 at 8:07 AM Martynas Pumputis <m@xxxxxxxxx> wrote:
>
>
>
> On 7/16/21 7:35 AM, Andrii Nakryiko wrote:
> > On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@xxxxxxxxx> wrote:
> >>
> >> Add a test case to check whether an unsuccessful creation of an outer
> >> map of a BTF-defined map-in-map destroys the inner map.
> >>
> >> As bpf_object__create_map() is a static function, we cannot just call it
> >> from the test case and then check whether a map accessible via
> >> map->inner_map_fd has been removed. Instead, we iterate over all maps
> >> and check whether the map "$MAP_NAME.inner" does not exist.
> >>
> >> Signed-off-by: Martynas Pumputis <m@xxxxxxxxx>
> >> ---
> >> .../bpf/progs/test_map_in_map_invalid.c | 27 +++++++++
> >> tools/testing/selftests/bpf/test_maps.c | 58 ++++++++++++++++++-
> >> 2 files changed, 84 insertions(+), 1 deletion(-)
> >> create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
> >>
> >> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
> >> new file mode 100644
> >> index 000000000000..03601779e4ed
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
> >> @@ -0,0 +1,27 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Copyright (c) 2021 Isovalent, Inc. */
> >> +#include <linux/bpf.h>
> >> +#include <bpf/bpf_helpers.h>
> >> +
> >> +struct inner {
> >> + __uint(type, BPF_MAP_TYPE_ARRAY);
> >> + __type(key, __u32);
> >> + __type(value, int);
> >> + __uint(max_entries, 4);
> >> +};
> >> +
> >> +struct {
> >> + __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> >> + __uint(max_entries, 0); /* This will make map creation to fail */
> >> + __uint(key_size, sizeof(__u32));
> >> + __array(values, struct inner);
> >> +} mim SEC(".maps");
> >> +
> >> +SEC("xdp_noop")
> >> +int xdp_noop0(struct xdp_md *ctx)
> >> +{
> >> + return XDP_PASS;
> >> +}
> >> +
> >> +int _version SEC("version") = 1;
> >
> > please don't add new uses of version, it's completely unnecessary on
> > modern kernels
>
> Sure.
>
> >
> >> +char _license[] SEC("license") = "GPL";
> >> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
> >> index 30cbf5d98f7d..48f6c6dfd188 100644
> >> --- a/tools/testing/selftests/bpf/test_maps.c
> >> +++ b/tools/testing/selftests/bpf/test_maps.c
> >> @@ -1153,12 +1153,16 @@ static void test_sockmap(unsigned int tasks, void *data)
> >> }
> >>
> >> #define MAPINMAP_PROG "./test_map_in_map.o"
> >> +#define MAPINMAP_INVALID_PROG "./test_map_in_map_invalid.o"
> >> static void test_map_in_map(void)
> >> {
> >> struct bpf_object *obj;
> >> struct bpf_map *map;
> >> int mim_fd, fd, err;
> >> int pos = 0;
> >> + struct bpf_map_info info = {};
> >> + __u32 len = sizeof(info);
> >> + __u32 id = 0;
> >>
> >> obj = bpf_object__open(MAPINMAP_PROG);
> >>
> >> @@ -1229,10 +1233,62 @@ static void test_map_in_map(void)
> >>
> >> close(fd);
> >> bpf_object__close(obj);
> >> +
> >> +
> >> + /* Test that failing bpf_object__create_map() destroys the inner map */
> >> +
> >> + obj = bpf_object__open(MAPINMAP_INVALID_PROG);
> >
> > you didn't check bpf_object__open() succeeded here...
>
> For the sake of brevity, I didn't add the check. If the opening fails,
> then we will catch it anyway with the bpf_object__find_map_by_name()
> invocation below: it will log "libbpf: elf: failed to open $PROG_NAME:
> No such file or directory" and then segfault.
Yeah, and then, due to the brevity you mentioned, someone like me will
go and waste time understanding what and where is crashing in our CIs.
Please add a proper check. Crashing test runners due to some
semi-expected failure is not an option. It happens in existing tests
very rarely, unfortunately, but it's a bug, not a feature.
>
> >
> >> +
> >> + map = bpf_object__find_map_by_name(obj, "mim");
> >
> > ... and crash will happen here on error
> >
> >> + if (!map) {
> >> + printf("Failed to load array of maps from test prog\n");
> >> + goto out_map_in_map;
> >> + }
> >> +
> >
> > [...]
> >
