| Message ID | 20210719173838.423148-3-m@lambda.lt (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: fix inner map removal in bpf_object__create_map | expand |
On Mon, Jul 19, 2021 at 10:36 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 closed. 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 | 26 ++++++++ > tools/testing/selftests/bpf/test_maps.c | 64 ++++++++++++++++++- > 2 files changed, 89 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..2918caea1e3d > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c > @@ -0,0 +1,26 @@ > +// 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") canonical section name for XDP programs is strictly "xdp", so I updated it to avoid fixing that later when libbpf will start enforcing this > +int xdp_noop0(struct xdp_md *ctx) > +{ > + return XDP_PASS; > +} > + > +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..d4184dde04df 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,68 @@ static void test_map_in_map(void) > > close(fd); added fd = -1 here > bpf_object__close(obj); > + > + > + /* Test that failing bpf_object__create_map() destroys the inner map */ > + > + obj = bpf_object__open(MAPINMAP_INVALID_PROG); > + err = libbpf_get_error(obj); > + if (err) { > + printf("Failed to load %s program: %d %d", > + MAPINMAP_INVALID_PROG, err, errno); > + goto out_map_in_map; > + } > + [...]
On Mon, Jul 19, 2021 at 10:36 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 closed. 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 | 26 ++++++++ > tools/testing/selftests/bpf/test_maps.c | 64 ++++++++++++++++++- > 2 files changed, 89 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c > [...] > + 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); Hi Martynas, This now is producing this warning, when running test_maps: libbpf: map 'mim': failed to create: Invalid argument(-22) libbpf: failed to load object './test_map_in_map_invalid.o' It's quite confusing, I think it's better to mute this. You can do that by temporarily swapping libbpf's logger function to a no-op function, ignoring all the warnings. We do this in few other places, see libbpf_set_print(). > + if (!err) { > + printf("Loading obj supposed to fail\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..2918caea1e3d --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c @@ -0,0 +1,26 @@ +// 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; +} + +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..d4184dde04df 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,68 @@ 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); + err = libbpf_get_error(obj); + if (err) { + printf("Failed to load %s program: %d %d", + MAPINMAP_INVALID_PROG, err, errno); + goto out_map_in_map; + } + + 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 closed. 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 | 26 ++++++++ tools/testing/selftests/bpf/test_maps.c | 64 ++++++++++++++++++- 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c