On Wed, Jan 3, 2024 at 12:57 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> Tbh, it looks like calls to zclose(map->fd) were unnecessary
> regardless of this patch, as all maps are closed at the end of
> bpf_object_load() in case of an error.
>
yep, agreed
> [...]
>
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index f29cfb344f80..e0085aef17d7 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
>
> [...]
>
> > @@ -5275,13 +5289,11 @@ static int bpf_object_create_map(struct bpf_object *obj, struct bpf_map *map, bo
> > create_attr.btf_value_type_id = 0;
> > map->btf_key_type_id = 0;
> > map->btf_value_type_id = 0;
> > - map->fd = bpf_map_create(def->type, map_name,
> > - def->key_size, def->value_size,
> > - def->max_entries, &create_attr);
> > + map_fd = bpf_map_create(def->type, map_name,
> > + def->key_size, def->value_size,
> > + def->max_entries, &create_attr);
> > }
> >
> > - err = map->fd < 0 ? -errno : 0;
> > -
> > if (bpf_map_type_is_map_in_map(def->type) && map->inner_map) {
> > if (obj->gen_loader)
> > map->inner_map->fd = -1;
> > @@ -5289,7 +5301,19 @@ static int bpf_object_create_map(struct bpf_object *obj, struct bpf_map *map, bo
> > zfree(&map->inner_map);
> > }
> >
> > - return err;
> > + if (map_fd < 0)
> > + return -errno;
>
> Nit: this check is now placed after call to bpf_map_destroy(),
> which might call munmap(), which might overwrite "errno",
> set by some of the previous calls to bpf_map_create().
>
this should be `return map_fd;`, no need for errno, good catch, fixed
> [...]
>
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index b5d334754e5d..662a3df1e29f 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -555,6 +555,30 @@ static inline int ensure_good_fd(int fd)
> > return fd;
> > }
> >
> > +static inline int create_placeholder_fd(void)
> > +{
> > + int fd;
> > +
> > + fd = ensure_good_fd(open("/dev/null", O_WRONLY | O_CLOEXEC));
>
> Stupid question: is it ok to assume that /dev is always mounted?
> Googling says that kernel chooses if to mount it automatically
> depending on the value of CONFIG_DEVTMPFS_MOUNT option.
> Another option might be memfd_create().
Yeah, good point, I actually don't know how reliable is /dev/null.
memfd_create() is not a bad idea, Linux 3.17+, should be fine. I'll
switch.
>
> > + if (fd < 0)
> > + return -errno;
> > + return fd;
> > +}
>
> [...]
>
>
