Re: [PATCH v2 bpf-next 06/17] libbpf: refactor BTF map definition parsing

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Apr 22, 2021 at 8:33 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
> > Refactor BTF-defined maps parsing logic to allow it to be nicely reused by BPF
> > static linker. Further, at least for BPF static linker, it's important to know
> > which attributes of a BPF map were defined explicitly, so provide a bit set
> > for each known portion of BTF map definition. This allows BPF static linker to
> > do a simple check when dealing with extern map declarations.
> >
> > The same capabilities allow to distinguish attributes explicitly set to zero
> > (e.g., __uint(max_entries, 0)) vs the case of not specifying it at all (no
> > max_entries attribute at all). Libbpf is currently not utilizing that, but it
> > could be useful for backwards compatibility reasons later.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >   tools/lib/bpf/libbpf.c          | 256 ++++++++++++++++++--------------
> >   tools/lib/bpf/libbpf_internal.h |  32 ++++
> >   2 files changed, 177 insertions(+), 111 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index a0e6d6bc47f3..f6f4126389ac 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -2025,255 +2025,262 @@ static int build_map_pin_path(struct bpf_map *map, const char *path)
> >       return bpf_map__set_pin_path(map, buf);
> >   }
> >
> > -
> > -static int parse_btf_map_def(struct bpf_object *obj,
> > -                          struct bpf_map *map,
> > -                          const struct btf_type *def,
> > -                          bool strict, bool is_inner,
> > -                          const char *pin_root_path)
> > +int parse_btf_map_def(const char *map_name, struct btf *btf,
> > +                   const struct btf_type *def_t, bool strict,
> > +                   struct btf_map_def *map_def, struct btf_map_def *inner_def)
> >   {
> >       const struct btf_type *t;
> >       const struct btf_member *m;
> > +     bool is_inner = inner_def == NULL;
> >       int vlen, i;
> >
> > -     vlen = btf_vlen(def);
> > -     m = btf_members(def);
> > +     vlen = btf_vlen(def_t);
> > +     m = btf_members(def_t);
> >       for (i = 0; i < vlen; i++, m++) {
> > -             const char *name = btf__name_by_offset(obj->btf, m->name_off);
> > +             const char *name = btf__name_by_offset(btf, m->name_off);
> >
> [...]
> >               }
> >               else if (strcmp(name, "values") == 0) {
> > +                     char inner_map_name[128];
> >                       int err;
> >
> >                       if (is_inner) {
> >                               pr_warn("map '%s': multi-level inner maps not supported.\n",
> > -                                     map->name);
> > +                                     map_name);
> >                               return -ENOTSUP;
> >                       }
> >                       if (i != vlen - 1) {
> >                               pr_warn("map '%s': '%s' member should be last.\n",
> > -                                     map->name, name);
> > +                                     map_name, name);
> >                               return -EINVAL;
> >                       }
> > -                     if (!bpf_map_type__is_map_in_map(map->def.type)) {
> > +                     if (!bpf_map_type__is_map_in_map(map_def->map_type)) {
> >                               pr_warn("map '%s': should be map-in-map.\n",
> > -                                     map->name);
> > +                                     map_name);
> >                               return -ENOTSUP;
> >                       }
> > -                     if (map->def.value_size && map->def.value_size != 4) {
> > +                     if (map_def->value_size && map_def->value_size != 4) {
> >                               pr_warn("map '%s': conflicting value size %u != 4.\n",
> > -                                     map->name, map->def.value_size);
> > +                                     map_name, map_def->value_size);
> >                               return -EINVAL;
> >                       }
> > -                     map->def.value_size = 4;
> > -                     t = btf__type_by_id(obj->btf, m->type);
> > +                     map_def->value_size = 4;
> > +                     t = btf__type_by_id(btf, m->type);
> >                       if (!t) {
> >                               pr_warn("map '%s': map-in-map inner type [%d] not found.\n",
> > -                                     map->name, m->type);
> > +                                     map_name, m->type);
> >                               return -EINVAL;
> >                       }
> >                       if (!btf_is_array(t) || btf_array(t)->nelems) {
> >                               pr_warn("map '%s': map-in-map inner spec is not a zero-sized array.\n",
> > -                                     map->name);
> > +                                     map_name);
> >                               return -EINVAL;
> >                       }
> > -                     t = skip_mods_and_typedefs(obj->btf, btf_array(t)->type,
> > -                                                NULL);
> > +                     t = skip_mods_and_typedefs(btf, btf_array(t)->type, NULL);
> >                       if (!btf_is_ptr(t)) {
> >                               pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> > -                                     map->name, btf_kind_str(t));
> > +                                     map_name, btf_kind_str(t));
> >                               return -EINVAL;
> >                       }
> > -                     t = skip_mods_and_typedefs(obj->btf, t->type, NULL);
> > +                     t = skip_mods_and_typedefs(btf, t->type, NULL);
> >                       if (!btf_is_struct(t)) {
> >                               pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> > -                                     map->name, btf_kind_str(t));
> > +                                     map_name, btf_kind_str(t));
> >                               return -EINVAL;
> >                       }
> >
> > -                     map->inner_map = calloc(1, sizeof(*map->inner_map));
> > -                     if (!map->inner_map)
> > -                             return -ENOMEM;
> > -                     map->inner_map->fd = -1;
> > -                     map->inner_map->sec_idx = obj->efile.btf_maps_shndx;
>
> The refactoring didn't set map->inner_map->sec_idx. I guess since
> inner_map is only used internally by libbpf, so it does not
> have a user visible sec_idx and hence useless? It is worthwhile to
> mention in the commit message for this difference, I think.
>
> > -                     map->inner_map->name = malloc(strlen(map->name) +
> > -                                                   sizeof(".inner") + 1);
> > -                     if (!map->inner_map->name)
> > -                             return -ENOMEM;
> > -                     sprintf(map->inner_map->name, "%s.inner", map->name);
> > -
> > -                     err = parse_btf_map_def(obj, map->inner_map, t, strict,
> > -                                             true /* is_inner */, NULL);
> > +                     snprintf(inner_map_name, sizeof(inner_map_name), "%s.inner", map_name);
> > +                     err = parse_btf_map_def(inner_map_name, btf, t, strict, inner_def, NULL);
> >                       if (err)
> >                               return err;
> > +
> > +                     map_def->parts |= MAP_DEF_INNER_MAP;
> >               } else if (strcmp(name, "pinning") == 0) {
> >                       __u32 val;
> > -                     int err;
> >
> >                       if (is_inner) {
> > -                             pr_debug("map '%s': inner def can't be pinned.\n",
> > -                                      map->name);
> > +                             pr_warn("map '%s': inner def can't be pinned.\n", map_name);
> >                               return -EINVAL;
> >                       }
> > -                     if (!get_map_field_int(map->name, obj->btf, m, &val))
> > +                     if (!get_map_field_int(map_name, btf, m, &val))
> >                               return -EINVAL;
> > -                     pr_debug("map '%s': found pinning = %u.\n",
> > -                              map->name, val);
> > -
> > -                     if (val != LIBBPF_PIN_NONE &&
> > -                         val != LIBBPF_PIN_BY_NAME) {
> > +                     if (val != LIBBPF_PIN_NONE && val != LIBBPF_PIN_BY_NAME) {
> >                               pr_warn("map '%s': invalid pinning value %u.\n",
> > -                                     map->name, val);
> > +                                     map_name, val);
> >                               return -EINVAL;
> >                       }
> > -                     if (val == LIBBPF_PIN_BY_NAME) {
> > -                             err = build_map_pin_path(map, pin_root_path);
> > -                             if (err) {
> > -                                     pr_warn("map '%s': couldn't build pin path.\n",
> > -                                             map->name);
> > -                                     return err;
> > -                             }
> > -                     }
> > +                     map_def->pinning = val;
> > +                     map_def->parts |= MAP_DEF_PINNING;
> >               } else {
> >                       if (strict) {
> > -                             pr_warn("map '%s': unknown field '%s'.\n",
> > -                                     map->name, name);
> > +                             pr_warn("map '%s': unknown field '%s'.\n", map_name, name);
> >                               return -ENOTSUP;
> >                       }
> > -                     pr_debug("map '%s': ignoring unknown field '%s'.\n",
> > -                              map->name, name);
> > +                     pr_debug("map '%s': ignoring unknown field '%s'.\n", map_name, name);
> >               }
> >       }
> >
> > -     if (map->def.type == BPF_MAP_TYPE_UNSPEC) {
> > -             pr_warn("map '%s': map type isn't specified.\n", map->name);
> > +     if (map_def->map_type == BPF_MAP_TYPE_UNSPEC) {
> > +             pr_warn("map '%s': map type isn't specified.\n", map_name);
> >               return -EINVAL;
> >       }
> >
> >       return 0;
> >   }
> >
> > +static void fill_map_from_def(struct bpf_map *map, const struct btf_map_def *def)
> > +{
> [...]
> > +}
> > +
> >   static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> >                                        const struct btf_type *sec,
> >                                        int var_idx, int sec_idx,
> >                                        const Elf_Data *data, bool strict,
> >                                        const char *pin_root_path)
> >   {
> > +     struct btf_map_def map_def = {}, inner_def = {};
> >       const struct btf_type *var, *def;
> >       const struct btf_var_secinfo *vi;
> >       const struct btf_var *var_extra;
> >       const char *map_name;
> >       struct bpf_map *map;
> > +     int err;
> >
> >       vi = btf_var_secinfos(sec) + var_idx;
> >       var = btf__type_by_id(obj->btf, vi->type);
> > @@ -2327,7 +2334,34 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> >       pr_debug("map '%s': at sec_idx %d, offset %zu.\n",
> >                map_name, map->sec_idx, map->sec_offset);
> >
> > -     return parse_btf_map_def(obj, map, def, strict, false, pin_root_path);
> > +     err = parse_btf_map_def(map->name, obj->btf, def, strict, &map_def, &inner_def);
> > +     if (err)
> > +             return err;
> > +
> > +     fill_map_from_def(map, &map_def);
> > +
> > +     if (map_def.pinning == LIBBPF_PIN_BY_NAME) {
> > +             err = build_map_pin_path(map, pin_root_path);
> > +             if (err) {
> > +                     pr_warn("map '%s': couldn't build pin path.\n", map->name);
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     if (map_def.parts & MAP_DEF_INNER_MAP) {
> > +             map->inner_map = calloc(1, sizeof(*map->inner_map));
> > +             if (!map->inner_map)
> > +                     return -ENOMEM;
> > +             map->inner_map->fd = -1;
>
> missing set map->inner_map->sec_idx here.

I'll add it back here, but it was never really necessary. More for the
completeness sake. sec_idx is used only to match relo to a map, and
this inner_map is never matched and never referenced by a relocation.

>
> > +             map->inner_map->name = malloc(strlen(map_name) + sizeof(".inner") + 1);
> > +             if (!map->inner_map->name)
> > +                     return -ENOMEM;
> > +             sprintf(map->inner_map->name, "%s.inner", map_name);
> > +
> > +             fill_map_from_def(map->inner_map, &inner_def);
> > +     }
> > +
> > +     return 0;
> >   }
> >
> >   static int bpf_object__init_user_btf_maps(struct bpf_object *obj, bool strict,
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 92b7eae10c6d..17883073710c 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -138,6 +138,38 @@ static inline __u32 btf_type_info(int kind, int vlen, int kflag)
> >       return (kflag << 31) | (kind << 24) | vlen;
> >   }
> >
> [...]