| Message ID | 20210714165440.472566-3-m@lambda.lt (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: fix inner map removal in bpf_object__create_map | expand |
Martynas Pumputis 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@lambda.lt> > --- Acked-by: John Fastabend <john.fastabend@gmail.com>
On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> 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@lambda.lt> > --- > .../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 > +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... > + > + 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; > + } > + [...]
On 7/16/21 7:35 AM, Andrii Nakryiko wrote: > On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> 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@lambda.lt> >> --- >> .../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. > >> + >> + 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; >> + } >> + > > [...] >
On Fri, Jul 16, 2021 at 8:07 AM Martynas Pumputis <m@lambda.lt> wrote: > > > > On 7/16/21 7:35 AM, Andrii Nakryiko wrote: > > On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> 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@lambda.lt> > >> --- > >> .../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; > >> + } > >> + > > > > [...] > >
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; +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); + + map = bpf_object__find_map_by_name(obj, "mim"); + if (!map) { + printf("Failed to load array of maps from test prog\n"); + goto out_map_in_map; + } + + err = bpf_object__load(obj); + if (!err) { + printf("Loading obj supposed to fail\n"); + goto out_map_in_map; + } + + /* Iterate over all maps to check whether the internal map + * ("mim.internal") has been destroyed. + */ + while (true) { + err = bpf_map_get_next_id(id, &id); + if (err) { + if (errno == ENOENT) + break; + printf("Failed to get next map: %d", errno); + goto out_map_in_map; + } + + fd = bpf_map_get_fd_by_id(id); + if (fd < 0) { + if (errno == ENOENT) + continue; + printf("Failed to get map by id %u: %d", id, errno); + goto out_map_in_map; + } + + err = bpf_obj_get_info_by_fd(fd, &info, &len); + if (err) { + printf("Failed to get map info by fd %d: %d", fd, + errno); + goto out_map_in_map; + } + + if (!strcmp(info.name, "mim.inner")) { + printf("Inner map mim.inner was not destroyed\n"); + goto out_map_in_map; + } + } + return; out_map_in_map: - close(fd); + if (fd >= 0) + close(fd); exit(1); }
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@lambda.lt> --- .../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