| Message ID | 20201002075750.1978298-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [libbpf] libbpf: check if pin_path was set even map fd exist | expand |
On Fri, Oct 02, 2020 at 03:57:50PM +0800, Hangbin Liu wrote: > Say a user reuse map fd after creating a map manually and set the > pin_path, then load the object via libbpf. > > In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will > return 0 if there is no pinned map in map->pin_path. Then after > checking if map fd exist, we should also check if pin_path was set > and do bpf_map__pin() instead of continue the loop. > > Fix it by creating map if fd not exist and continue checking pin_path > after that. > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++--------------------- > 1 file changed, 37 insertions(+), 38 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index e493d6048143..d4149585a76c 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -3861,50 +3861,49 @@ bpf_object__create_maps(struct bpf_object *obj) > } > } > > - if (map->fd >= 0) { > - pr_debug("map '%s': skipping creation (preset fd=%d)\n", > - map->name, map->fd); > - continue; > - } > - > - err = bpf_object__create_map(obj, map); > - if (err) > - goto err_out; > - > - pr_debug("map '%s': created successfully, fd=%d\n", map->name, > - map->fd); > - > - if (bpf_map__is_internal(map)) { > - err = bpf_object__populate_internal_map(obj, map); > - if (err < 0) { > - zclose(map->fd); > + if (map->fd < 0) { > + err = bpf_object__create_map(obj, map); > + if (err) > goto err_out; > - } > - } > - > - if (map->init_slots_sz) { > - for (j = 0; j < map->init_slots_sz; j++) { > - const struct bpf_map *targ_map; > - int fd; > > - if (!map->init_slots[j]) > - continue; > + pr_debug("map '%s': created successfully, fd=%d\n", map->name, > + map->fd); > > - targ_map = map->init_slots[j]; > - fd = bpf_map__fd(targ_map); > - err = bpf_map_update_elem(map->fd, &j, &fd, 0); > - if (err) { > - err = -errno; > - pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n", > - map->name, j, targ_map->name, > - fd, err); > + if (bpf_map__is_internal(map)) { > + err = bpf_object__populate_internal_map(obj, map); > + if (err < 0) { > + zclose(map->fd); > goto err_out; > } > - pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n", > - map->name, j, targ_map->name, fd); > } > - zfree(&map->init_slots); > - map->init_slots_sz = 0; > + > + if (map->init_slots_sz) { Couldn't we flatten the code by inverting the logic here and using goto? if (!map->init_slot_sz) { pr_debug("map '%s': skipping creation (preset fd=%d)\n", map->name, map->fd); goto map_pin; } (...) map_pin: if (map->pin_path && !map->pinned) { If I'm reading this right. > + for (j = 0; j < map->init_slots_sz; j++) { > + const struct bpf_map *targ_map; > + int fd; > + > + if (!map->init_slots[j]) > + continue; > + > + targ_map = map->init_slots[j]; > + fd = bpf_map__fd(targ_map); > + err = bpf_map_update_elem(map->fd, &j, &fd, 0); > + if (err) { > + err = -errno; > + pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n", > + map->name, j, targ_map->name, > + fd, err); > + goto err_out; > + } > + pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n", > + map->name, j, targ_map->name, fd); > + } > + zfree(&map->init_slots); > + map->init_slots_sz = 0; > + } > + } else { > + pr_debug("map '%s': skipping creation (preset fd=%d)\n", > + map->name, map->fd); > } > > if (map->pin_path && !map->pinned) { > -- > 2.25.4 >
On Fri, Oct 2, 2020 at 12:58 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > Say a user reuse map fd after creating a map manually and set the > pin_path, then load the object via libbpf. > > In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will > return 0 if there is no pinned map in map->pin_path. Then after > checking if map fd exist, we should also check if pin_path was set > and do bpf_map__pin() instead of continue the loop. > > Fix it by creating map if fd not exist and continue checking pin_path > after that. > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++--------------------- > 1 file changed, 37 insertions(+), 38 deletions(-) > Please add a selftests that validates the logic you are going to rely on. > + targ_map = map->init_slots[j]; > + fd = bpf_map__fd(targ_map); > + err = bpf_map_update_elem(map->fd, &j, &fd, 0); > + if (err) { > + err = -errno; > + pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n", > + map->name, j, targ_map->name, > + fd, err); I just noticed that we don't zclose(map->fd) here, can you please fix it with a separate patch along these changes? Thanks! > + goto err_out; > + } > + pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n", > + map->name, j, targ_map->name, fd); > + } > + zfree(&map->init_slots); > + map->init_slots_sz = 0; Let's move this slot initting logic into a helper function (init_map_slots() or something like that? doesn't have to use "bpf_object__" prefix as it is internal static function), that will simplify overall flow. > + } > + } else { > + pr_debug("map '%s': skipping creation (preset fd=%d)\n", > + map->name, map->fd); to make diff a bit smaller, maybe let's keep the original order, but do if/else instead of continuing: if (map->fd >= 0) { pr_debug("skipping..."); } else { /* do the creation here */ } /* pinning logic here */ > } > > if (map->pin_path && !map->pinned) { > -- > 2.25.4 >
On Fri, Oct 2, 2020 at 4:56 AM Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: > > On Fri, Oct 02, 2020 at 03:57:50PM +0800, Hangbin Liu wrote: > > Say a user reuse map fd after creating a map manually and set the > > pin_path, then load the object via libbpf. > > > > In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will > > return 0 if there is no pinned map in map->pin_path. Then after > > checking if map fd exist, we should also check if pin_path was set > > and do bpf_map__pin() instead of continue the loop. > > > > Fix it by creating map if fd not exist and continue checking pin_path > > after that. > > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > --- > > tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++--------------------- > > 1 file changed, 37 insertions(+), 38 deletions(-) > > [...] > > + > > + if (map->init_slots_sz) { > > Couldn't we flatten the code by inverting the logic here and using goto? I explicitly asked Hangbin to not use goto to alter control flow here, I'd like to keep goto within libbpf mostly for error handling. > > if (!map->init_slot_sz) { > pr_debug("map '%s': skipping creation (preset fd=%d)\n", > map->name, map->fd); > goto map_pin; > } > > (...) > map_pin: > if (map->pin_path && !map->pinned) { > > If I'm reading this right. > [...] > > -- > > 2.25.4 > >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index e493d6048143..d4149585a76c 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -3861,50 +3861,49 @@ bpf_object__create_maps(struct bpf_object *obj) } } - if (map->fd >= 0) { - pr_debug("map '%s': skipping creation (preset fd=%d)\n", - map->name, map->fd); - continue; - } - - err = bpf_object__create_map(obj, map); - if (err) - goto err_out; - - pr_debug("map '%s': created successfully, fd=%d\n", map->name, - map->fd); - - if (bpf_map__is_internal(map)) { - err = bpf_object__populate_internal_map(obj, map); - if (err < 0) { - zclose(map->fd); + if (map->fd < 0) { + err = bpf_object__create_map(obj, map); + if (err) goto err_out; - } - } - - if (map->init_slots_sz) { - for (j = 0; j < map->init_slots_sz; j++) { - const struct bpf_map *targ_map; - int fd; - if (!map->init_slots[j]) - continue; + pr_debug("map '%s': created successfully, fd=%d\n", map->name, + map->fd); - targ_map = map->init_slots[j]; - fd = bpf_map__fd(targ_map); - err = bpf_map_update_elem(map->fd, &j, &fd, 0); - if (err) { - err = -errno; - pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n", - map->name, j, targ_map->name, - fd, err); + if (bpf_map__is_internal(map)) { + err = bpf_object__populate_internal_map(obj, map); + if (err < 0) { + zclose(map->fd); goto err_out; } - pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n", - map->name, j, targ_map->name, fd); } - zfree(&map->init_slots); - map->init_slots_sz = 0; + + if (map->init_slots_sz) { + for (j = 0; j < map->init_slots_sz; j++) { + const struct bpf_map *targ_map; + int fd; + + if (!map->init_slots[j]) + continue; + + targ_map = map->init_slots[j]; + fd = bpf_map__fd(targ_map); + err = bpf_map_update_elem(map->fd, &j, &fd, 0); + if (err) { + err = -errno; + pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n", + map->name, j, targ_map->name, + fd, err); + goto err_out; + } + pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n", + map->name, j, targ_map->name, fd); + } + zfree(&map->init_slots); + map->init_slots_sz = 0; + } + } else { + pr_debug("map '%s': skipping creation (preset fd=%d)\n", + map->name, map->fd); } if (map->pin_path && !map->pinned) {
Say a user reuse map fd after creating a map manually and set the pin_path, then load the object via libbpf. In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will return 0 if there is no pinned map in map->pin_path. Then after checking if map fd exist, we should also check if pin_path was set and do bpf_map__pin() instead of continue the loop. Fix it by creating map if fd not exist and continue checking pin_path after that. Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 38 deletions(-)