| Message ID | 20210714165440.472566-2-m@lambda.lt (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: fix inner map removal in bpf_object__create_map | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: toke@redhat.com; 7 maintainers not CCed: netdev@vger.kernel.org yhs@fb.com kpsingh@kernel.org kafai@fb.com john.fastabend@gmail.com songliubraving@fb.com toke@redhat.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 23 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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@kernel.org> > Signed-off-by: Martynas Pumputis <m@lambda.lt> > --- > 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. > 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! > 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 >
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@kernel.org> >> Signed-off-by: Martynas Pumputis <m@lambda.lt> >> --- >> 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@gmail.com/. > > 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 >> > >
Martynas Pumputis wrote: > > > 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@kernel.org> > >> Signed-off-by: Martynas Pumputis <m@lambda.lt> > >> --- > >> 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. 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. > > > >> 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@gmail.com/. +1 > > > > > Thanks! > > > >
John Fastabend wrote: > Martynas Pumputis wrote: > > > > > > 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@kernel.org> > > >> Signed-off-by: Martynas Pumputis <m@lambda.lt> > > >> --- > > >> 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. > > 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 ;)
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@kernel.org> > Signed-off-by: Martynas Pumputis <m@lambda.lt> > --- LGTM. Acked-by: John Fastabend <john.fastabend@gmail.com>
On Wed, Jul 14, 2021 at 9:52 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> > Signed-off-by: Martynas Pumputis <m@lambda.lt> > --- > 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; 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. > > 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 >
On 7/16/21 7:27 AM, Andrii Nakryiko wrote: > On Wed, Jul 14, 2021 at 9:52 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> >> Signed-off-by: Martynas Pumputis <m@lambda.lt> >> --- >> 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; > > 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. Good idea. I will send v2 once we have agreed on the selftesting issue. > >> >> 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 >>
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; 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)
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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)