[PATCH v2 bpf 0/2] libbpf: fix inner map removal in bpf_object__create_map

[PATCH v2 bpf 0/2] libbpf: fix inner map removal in bpf_object__create_map

[Martynas Pumputis]

This patch set fixes the removal of a BTF-defined inner map if the
creation of the outer map has failed. This was identified by Andrii
in [1].

[1]: https://lore.kernel.org/bpf/CAEf4BzYaQsD6NaEUij6ttDeKYP7oEB0=c0D9_xdAKw6FYb7h1g@mail.gmail.com/



Martynas Pumputis (2):
  libbpf: fix removal of inner map in bpf_object__create_map
  selftests/bpf: check inner map deletion

 tools/lib/bpf/libbpf.c                        | 11 ++--
 .../bpf/progs/test_map_in_map_invalid.c       | 26 ++++++++
 tools/testing/selftests/bpf/test_maps.c       | 64 ++++++++++++++++++-
 3 files changed, 95 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c

-- 
2.32.0

[PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[Martynas Pumputis]

If creating an outer map of a BTF-defined map-in-map fails (via
bpf_object__create_map()), then the previously created its inner map
won't be destroyed.

Fix this by ensuring that the destroy routines are not bypassed in the
case of a failure.

Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
---
 tools/lib/bpf/libbpf.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6f5e2757bb3c..dde521366579 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 {
 	struct bpf_create_map_attr create_attr;
 	struct bpf_map_def *def = &map->def;
+	int err = 0;
 
 	memset(&create_attr, 0, sizeof(create_attr));
 
@@ -4521,8 +4522,6 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 
 	if (bpf_map_type__is_map_in_map(def->type)) {
 		if (map->inner_map) {
-			int err;
-
 			err = bpf_object__create_map(obj, map->inner_map, true);
 			if (err) {
 				pr_warn("map '%s': failed to create inner map: %d\n",
@@ -4547,7 +4546,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 	if (map->fd < 0 && (create_attr.btf_key_type_id ||
 			    create_attr.btf_value_type_id)) {
 		char *cp, errmsg[STRERR_BUFSIZE];
-		int err = -errno;
+		err = -errno;
 
 		cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
 		pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
@@ -4560,8 +4559,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 		map->fd = bpf_create_map_xattr(&create_attr);
 	}
 
-	if (map->fd < 0)
-		return -errno;
+	err = map->fd < 0 ? -errno : 0;
 
 	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
 		if (obj->gen_loader)
@@ -4570,7 +4568,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 		zfree(&map->inner_map);
 	}
 
-	return 0;
+	return err;
 }
 
 static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
-- 
2.32.0

[PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Martynas Pumputis]

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")
+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);
 }
 
-- 
2.32.0

Re: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[Andrii Nakryiko]

On Mon, Jul 19, 2021 at 10:36 AM Martynas Pumputis <m@lambda.lt> wrote:
>
> If creating an outer map of a BTF-defined map-in-map fails (via
> bpf_object__create_map()), then the previously created its inner map
> won't be destroyed.
>
> Fix this by ensuring that the destroy routines are not bypassed in the
> case of a failure.
>
> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>

Please preserve received acks between versions (unless some
significant changes happen between versions invalidating the original
ack):

Acked-by: John Fastabend <john.fastabend@gmail.com>

Thanks, applied (with few adjustments) to bpf-next, given we lived
with this bug for a long while and no one ever noticed/reported it
(and BPF map will be cleaned up when the process exits anyway).
Keeping it in bpf-next will make it easier for follow-up patches
changing/enhancing bpf_map__create_map() and will avoid merge
conflicts down the line.

> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> ---
>  tools/lib/bpf/libbpf.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6f5e2757bb3c..dde521366579 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>  {
>         struct bpf_create_map_attr create_attr;
>         struct bpf_map_def *def = &map->def;
> +       int err = 0;
>
>         memset(&create_attr, 0, sizeof(create_attr));
>
> @@ -4521,8 +4522,6 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>
>         if (bpf_map_type__is_map_in_map(def->type)) {
>                 if (map->inner_map) {
> -                       int err;
> -
>                         err = bpf_object__create_map(obj, map->inner_map, true);
>                         if (err) {
>                                 pr_warn("map '%s': failed to create inner map: %d\n",
> @@ -4547,7 +4546,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>         if (map->fd < 0 && (create_attr.btf_key_type_id ||
>                             create_attr.btf_value_type_id)) {
>                 char *cp, errmsg[STRERR_BUFSIZE];
> -               int err = -errno;

needed empty line here


> +               err = -errno;
>
>                 cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
>                 pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
> @@ -4560,8 +4559,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>                 map->fd = bpf_create_map_xattr(&create_attr);
>         }
>
> -       if (map->fd < 0)
> -               return -errno;
> +       err = map->fd < 0 ? -errno : 0;
>
>         if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
>                 if (obj->gen_loader)
> @@ -4570,7 +4568,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>                 zfree(&map->inner_map);
>         }
>
> -       return 0;
> +       return err;
>  }
>
>  static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> --
> 2.32.0
>

Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Andrii Nakryiko]

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;
> +       }
> +

[...]

Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Andrii Nakryiko]

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;
> +       }
> +

[...]

Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Andrii Nakryiko]

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;
> >> +       }
> >> +
> >
> > [...]
> >

Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Martynas Pumputis]

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;
>> +       }
>> +
> 
> [...]
> 

Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Andrii Nakryiko]

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;
> +       }
> +

[...]

RE: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[John Fastabend]

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>

[PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Martynas Pumputis]

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;
+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);
 }
 
-- 
2.32.0