[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

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
>

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

[...]