libbpf: fix inner map removal in bpf_object__create_map

PATCH Superseded Martynas Pumputis 3 participants 14 messages Jul 14, 2021 – Jul 16, 2021

Subsystems (4)

Kselftest BPF BPF selftests libbpf

lore

Share & tools

lore

Martynas Pumputis

author

2021-07-14 16:52 UTC #0 lore raw

cover libbpf: fix inner map removal in bpf_object__create_map

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                        |  5 +-
 .../bpf/progs/test_map_in_map_invalid.c       | 27 +++++++++
 tools/testing/selftests/bpf/test_maps.c       | 58 ++++++++++++++++++-
 3 files changed, 87 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c

Signature · 1 line

2.32.0

Martynas Pumputis

author

2021-07-14 16:52 UTC #1 lore raw

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

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 <[email protected]>

Signed-off-by Martynas Pumputis <[email protected]>

---
 tools/lib/bpf/libbpf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

tools/lib/bpf/libbpf.c +3 -2

index 6f5e2757bb3c..1a840e81ea0a 100644
@@ -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 ret = 0;
 
 	memset(&create_attr, 0, sizeof(create_attr));

@@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 	}
 
 	if (map->fd < 0)
-		return -errno;
+		ret = -errno;
 
 	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
 		if (obj->gen_loader)

@@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 		zfree(&map->inner_map);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)

Signature · 1 line

2.32.0

Martynas Pumputis

author

2021-07-14 16:52 UTC #9 lore raw

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

In reply to 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 <[email protected]>

---
 .../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

tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +27 -0

new file mode 100644
index 000000000000..03601779e4ed
@@ -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";

tools/testing/selftests/bpf/test_maps.c +57 -1

79 lines not on this page · raw message

Signature · 1 line

2.32.0

John Fastabend

2021-07-15 00:30 UTC #2 lore raw

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

In reply to Martynas Pumputis

Quoted 27 lines · 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 <[email protected]>
Signed-off-by Martynas Pumputis <[email protected]>
---
 tools/lib/bpf/libbpf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
tools/lib/bpf/libbpf.c +1 -0
index 6f5e2757bb3c..1a840e81ea0a 100644
@@ -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 ret = 0;
 
 	memset(&create_attr, 0, sizeof(create_attr));
 

tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4566

Quoted 6 lines

tools/lib/bpf/libbpf.c +1 -1

@@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 	}
 
 	if (map->fd < 0)
-		return -errno;
+		ret = -errno;

I'm trying to track this down, not being overly familiar with this bit of code.

We entered bpf_object__create_map with map->inner_map potentially set and then here we are going to zfree(&map->inner_map). I'm trying to track down if this is problematic, I guess not? But seems like we could also free a map here that we didn't create from this call in the above logic.

tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4567

Quoted 1 line

tools/lib/bpf/libbpf.c +0 -0

 	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {

        if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
                if (obj->gen_loader)
                        map->inner_map->fd = -1;
                bpf_map__destroy(map->inner_map);
                zfree(&map->inner_map);
        }

Also not sure here, sorry didn't have time to follow too thoroughly will check again later. But, the 'map->inner_map->fd = -1' is going to cause bpf_map__destroy to bypass the close(fd) as I understand it. So are we leaking an fd if the inner_map->fd is coming from above create? Or maybe never happens because obj->gen_loader is NULL?

Thanks!

Quoted 13 lines

tools/lib/bpf/libbpf.c +1 -1

 		if (obj->gen_loader)

@@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 		zfree(&map->inner_map);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)

Signature · 1 line

2.32.0

Martynas Pumputis

author

2021-07-15 10:04 UTC #3 lore raw

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

Quoted 38 lines · John Fastabend

Quoted 27 lines · 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 <[email protected]>
Signed-off-by Martynas Pumputis <[email protected]>
---
  tools/lib/bpf/libbpf.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
tools/lib/bpf/libbpf.c +1 -0
index 6f5e2757bb3c..1a840e81ea0a 100644
@@ -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 ret = 0;
 
  	memset(&create_attr, 0, sizeof(create_attr));
 
tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4566
Quoted 6 lines
tools/lib/bpf/libbpf.c +1 -1
@@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
  	}
 
  	if (map->fd < 0)
-		return -errno;
+		ret = -errno;
 
I'm trying to track this down, not being overly familiar with this bit of code.

Quoted 6 lines

We entered bpf_object__create_map with map->inner_map potentially set and then here we are going to zfree(&map->inner_map). I'm trying to track down if this is problematic, I guess not? But seems like we could also free a map here that we didn't create from this call in the above logic.

Keep in mind that we free the inner map anyway if the creation of the outer map was successful. Also, we don't try to recreate the map if any of the steps has failed. So I think it should not be problematic.

Quoted 10 lines

tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4567

Quoted 1 line

tools/lib/bpf/libbpf.c +0 -0

  	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {

         if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
                 if (obj->gen_loader)
                         map->inner_map->fd = -1;
                 bpf_map__destroy(map->inner_map);
                 zfree(&map->inner_map);
         }

Quoted 5 lines

Also not sure here, sorry didn't have time to follow too thoroughly will check again later. But, the 'map->inner_map->fd = -1' is going to cause bpf_map__destroy to bypass the close(fd) as I understand it. So are we leaking an fd if the inner_map->fd is coming from above create? Or maybe never happens because obj->gen_loader is NULL?

I think in the case of obj->gen_loader, we don't need to close the FD of any map, as the creation of maps will happen at a later stage in the kernel: https://lore.kernel.org/bpf/[email protected]/.

Quoted 19 lines

Thanks!

Quoted 13 lines

tools/lib/bpf/libbpf.c +1 -1

  		if (obj->gen_loader)

@@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
  		zfree(&map->inner_map);
  	}
 
-	return 0;
+	return ret;
  }
 
  static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)

Signature · 1 line

2.32.0

John Fastabend

2021-07-15 20:04 UTC #4 lore raw

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

Quoted 48 lines · Martynas Pumputis

Quoted 38 lines · John Fastabend
Quoted 27 lines · 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 <[email protected]>
Signed-off-by Martynas Pumputis <[email protected]>
---
  tools/lib/bpf/libbpf.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
tools/lib/bpf/libbpf.c +1 -0
index 6f5e2757bb3c..1a840e81ea0a 100644
@@ -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 ret = 0;
 
  	memset(&create_attr, 0, sizeof(create_attr));
 
tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4566
Quoted 6 lines
tools/lib/bpf/libbpf.c +1 -1
@@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
  	}
 
  	if (map->fd < 0)
-		return -errno;
+		ret = -errno;
 
I'm trying to track this down, not being overly familiar with this bit of code.
Quoted 6 lines
We entered bpf_object__create_map with map->inner_map potentially set and then here we are going to zfree(&map->inner_map). I'm trying to track down if this is problematic, I guess not? But seems like we could also free a map here that we didn't create from this call in the above logic.

Quoted 3 lines

Keep in mind that we free the inner map anyway if the creation of the outer map was successful. Also, we don't try to recreate the map if any of the steps has failed. So I think it should not be problematic.

Maybe not problematic, but I'm still missing a bit of detail here. We create an inner map then immediately destroy it? I'll need to reread to understand the why part.

Quoted 18 lines

Quoted 12 lines
tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4567
Quoted 3 lines
tools/lib/bpf/libbpf.c +0 -0
  	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
         if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
                 if (obj->gen_loader)
                         map->inner_map->fd = -1;
                 bpf_map__destroy(map->inner_map);
                 zfree(&map->inner_map);
         }
Quoted 5 lines
Also not sure here, sorry didn't have time to follow too thoroughly will check again later. But, the 'map->inner_map->fd = -1' is going to cause bpf_map__destroy to bypass the close(fd) as I understand it. So are we leaking an fd if the inner_map->fd is coming from above create? Or maybe never happens because obj->gen_loader is NULL?

Quoted 4 lines

I think in the case of obj->gen_loader, we don't need to close the FD of any map, as the creation of maps will happen at a later stage in the kernel:
https://lore.kernel.org/bpf/[email protected]/.

Quoted 5 lines

Quoted 5 lines
Thanks!

John Fastabend

2021-07-16 03:07 UTC #5 lore raw

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

Quoted 49 lines · John Fastabend

Quoted 48 lines · Martynas Pumputis
Quoted 44 lines · John Fastabend
Quoted 27 lines · 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 <[email protected]>
Signed-off-by Martynas Pumputis <[email protected]>
---
  tools/lib/bpf/libbpf.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
tools/lib/bpf/libbpf.c +1 -0
index 6f5e2757bb3c..1a840e81ea0a 100644
@@ -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 ret = 0;
 
  	memset(&create_attr, 0, sizeof(create_attr));
 
tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4566
Quoted 6 lines
tools/lib/bpf/libbpf.c +1 -1
@@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
  	}
 
  	if (map->fd < 0)
-		return -errno;
+		ret = -errno;
 
I'm trying to track this down, not being overly familiar with this bit of code.
We entered bpf_object__create_map with map->inner_map potentially set and then here we are going to zfree(&map->inner_map). I'm trying to track down if this is problematic, I guess not? But seems like we could also free a map here that we didn't create from this call in the above logic.

Quoted 7 lines

Quoted 3 lines
Keep in mind that we free the inner map anyway if the creation of the outer map was successful. Also, we don't try to recreate the map if any of the steps has failed. So I think it should not be problematic.
Maybe not problematic, but I'm still missing a bit of detail here. We create an inner map then immediately destroy it? I'll need to reread to understand the why part.

OK understand now ;)

John Fastabend

Acked-by on v1 1/2

2021-07-16 03:09 UTC #6 lore raw

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

In reply to Martynas Pumputis

Quoted 11 lines · 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 <[email protected]>
Signed-off-by Martynas Pumputis <[email protected]>
---

LGTM.

Acked-by John Fastabend <[email protected]>

John Fastabend

Acked-by on v1 2/2

2021-07-16 03:11 UTC #10 lore raw

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

In reply to Martynas Pumputis

Quoted 10 lines · 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 <[email protected]>
---

Acked-by John Fastabend <[email protected]>

Andrii Nakryiko

2021-07-16 05:27 UTC #7 lore raw

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

In reply to Martynas Pumputis

Quoted 28 lines · 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 <[email protected]>
Signed-off-by Martynas Pumputis <[email protected]>
---
 tools/lib/bpf/libbpf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
tools/lib/bpf/libbpf.c +1 -0
index 6f5e2757bb3c..1a840e81ea0a 100644
@@ -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 ret = 0;
 
        memset(&create_attr, 0, sizeof(create_attr));
 

tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4565

Quoted 5 lines

tools/lib/bpf/libbpf.c +1 -1

@@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
        }
 
        if (map->fd < 0)
-               return -errno;
+               ret = -errno;

Oh, isn't this a complicated function, eh? I stared at the code for a while until I understood the whole idea with map->inner_map handling there.

I think your change is correct, I'd just love you to consolidate all those "int err" definitions, and use just one throughout this function. It will clean up two other if() blocks, and in this case "err" name is more appropriate, because it always is <= 0.

Quoted 15 lines

tools/lib/bpf/libbpf.c +1 -2

        if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
                if (obj->gen_loader)

@@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
                zfree(&map->inner_map);
        }
 
-       return 0;
+       return ret;
 }
 
 static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
--

2.32.0

Andrii Nakryiko

2021-07-16 05:35 UTC #11 lore raw

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

In reply to Martynas Pumputis

Quoted 38 lines · 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 <[email protected]>
---
 .../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
tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +16 -0
new file mode 100644
index 000000000000..03601779e4ed
@@ -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));

tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c line 26

Quoted 10 lines

tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +10 -0

+       __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

Quoted 23 lines

tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +1 -0

+char _license[] SEC("license") = "GPL";

tools/testing/selftests/bpf/test_maps.c +4 -0

index 30cbf5d98f7d..48f6c6dfd188 100644
@@ -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);

tools/testing/selftests/bpf/test_maps.c static void test_map_in_map(void) · line 1240

Quoted 8 lines

tools/testing/selftests/bpf/test_maps.c +5 -0

@@ -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...

tools/testing/selftests/bpf/test_maps.c static void test_map_in_map(void) · line 1242

Quoted 2 lines

tools/testing/selftests/bpf/test_maps.c +2 -0
+
+       map = bpf_object__find_map_by_name(obj, "mim");

... and crash will happen here on error

Quoted 5 lines

tools/testing/selftests/bpf/test_maps.c +5 -0

+       if (!map) {
+               printf("Failed to load array of maps from test prog\n");
+               goto out_map_in_map;
+       }
+

[...]

Martynas Pumputis

author

2021-07-16 15:07 UTC #12 lore raw

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

Quoted 44 lines · Andrii Nakryiko

Quoted 43 lines · 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 <[email protected]>
---
  .../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
tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +21 -0
new file mode 100644
index 000000000000..03601779e4ed
@@ -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)

Quoted 8 lines

tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c line 26
Quoted 5 lines
tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +5 -0
+{
+       return XDP_PASS;
+}
+
+int _version SEC("version") = 1;
please don't add new uses of version, it's completely unnecessary on modern kernels

Sure.

Quoted 25 lines

tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +1 -0

+char _license[] SEC("license") = "GPL";

tools/testing/selftests/bpf/test_maps.c +4 -0

index 30cbf5d98f7d..48f6c6dfd188 100644
@@ -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)

Quoted 9 lines

tools/testing/selftests/bpf/test_maps.c static void test_map_in_map(void) · line 1240
Quoted 7 lines
tools/testing/selftests/bpf/test_maps.c +5 -0
         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.

Quoted 14 lines

tools/testing/selftests/bpf/test_maps.c static void test_map_in_map(void) · line 1242
Quoted 3 lines
tools/testing/selftests/bpf/test_maps.c +2 -0
+
+       map = bpf_object__find_map_by_name(obj, "mim");
... and crash will happen here on error
Quoted 5 lines
tools/testing/selftests/bpf/test_maps.c +5 -0
+       if (!map) {
+               printf("Failed to load array of maps from test prog\n");
+               goto out_map_in_map;
+       }
+
[...]

Martynas Pumputis

author

2021-07-16 15:22 UTC #8 lore raw

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

In reply to Andrii Nakryiko

Quoted 39 lines · Andrii Nakryiko

Quoted 28 lines · 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 <[email protected]>
Signed-off-by Martynas Pumputis <[email protected]>
---
  tools/lib/bpf/libbpf.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
tools/lib/bpf/libbpf.c +1 -0
index 6f5e2757bb3c..1a840e81ea0a 100644
@@ -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 ret = 0;
 
         memset(&create_attr, 0, sizeof(create_attr));
 
tools/lib/bpf/libbpf.c static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b · line 4565
Quoted 5 lines
tools/lib/bpf/libbpf.c +1 -1
@@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
         }
 
         if (map->fd < 0)
-               return -errno;
+               ret = -errno;
Oh, isn't this a complicated function, eh? I stared at the code for a while until I understood the whole idea with map->inner_map handling there.

Quoted 4 lines

I think your change is correct, I'd just love you to consolidate all those "int err" definitions, and use just one throughout this function. It will clean up two other if() blocks, and in this case "err" name is more appropriate, because it always is <= 0.

Good idea. I will send v2 once we have agreed on the selftesting issue.

Quoted 16 lines

tools/lib/bpf/libbpf.c +1 -2

         if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
                 if (obj->gen_loader)

@@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
                 zfree(&map->inner_map);
         }
 
-       return 0;
+       return ret;
  }
 
  static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
--

2.32.0

Andrii Nakryiko

2021-07-16 18:24 UTC #13 lore raw

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

In reply to Martynas Pumputis

Quoted 94 lines · Martynas Pumputis

Quoted 44 lines · Andrii Nakryiko
Quoted 43 lines · 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 <[email protected]>
---
  .../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
tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +21 -0
new file mode 100644
index 000000000000..03601779e4ed
@@ -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)
Quoted 8 lines
tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c line 26
Quoted 5 lines
tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +5 -0
+{
+       return XDP_PASS;
+}
+
+int _version SEC("version") = 1;
please don't add new uses of version, it's completely unnecessary on modern kernels
Sure.
Quoted 25 lines
Quoted 25 lines
tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c new file +1 -0
+char _license[] SEC("license") = "GPL";
tools/testing/selftests/bpf/test_maps.c +4 -0
index 30cbf5d98f7d..48f6c6dfd188 100644
@@ -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)
 
Quoted 9 lines
tools/testing/selftests/bpf/test_maps.c static void test_map_in_map(void) · line 1240
Quoted 7 lines
tools/testing/selftests/bpf/test_maps.c +5 -0
         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...

Quoted 4 lines

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.

Quoted 15 lines

Quoted 15 lines
tools/testing/selftests/bpf/test_maps.c static void test_map_in_map(void) · line 1242
Quoted 4 lines
tools/testing/selftests/bpf/test_maps.c +2 -0
+
+       map = bpf_object__find_map_by_name(obj, "mim");
... and crash will happen here on error
Quoted 5 lines
tools/testing/selftests/bpf/test_maps.c +5 -0
+       if (!map) {
+               printf("Failed to load array of maps from test prog\n");
+               goto out_map_in_map;
+       }
+
[...]