On 7/15/21 2:30 AM, John Fastabend wrote:
Martynas Pumputis 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@xxxxxxxxxx>
Signed-off-by: Martynas Pumputis <m@xxxxxxxxx>
---
tools/lib/bpf/libbpf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6f5e2757bb3c..1a840e81ea0a 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 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;
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.
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.
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?
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/20210514003623.28033-15-alexei.starovoitov@xxxxxxxxx/.
Thanks!
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
