Re: [PATCH v2 bpf-next 11/13] libbpf: Make BTF mandatory if program BTF has spin_lock or alloc_obj type

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

On Sat, Dec 17, 2022 at 12:25 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> If a BPF program defines a struct or union type which has a field type
> that the verifier considers special - spin_lock, graph datastructure
> heads and nodes - the verifier needs to be able to find fields of that
> type using BTF.
>
> For such a program, BTF is required, so modify kernel_needs_btf helper
> to ensure that correct "BTF is mandatory" error message is emitted.
>
> The newly-added btf_has_alloc_obj_type looks for BTF_KIND_STRUCTs with a
> name corresponding to a special type. If any such struct is found it is
> assumed that some variable is using it, and therefore that successful
> BTF load is necessary.
>
> Also add a kernel_needs_btf check to bpf_object__create_map where it was
> previously missing. When this function calls bpf_map_create, kernel may
> reject map creation due to mismatched graph owner and ownee
> types (e.g. a struct bpf_list_head with __contains tag pointing to
> bpf_rbtree_node field). In such a scenario - or any other where BTF is
> necessary for verification - bpf_map_create should not be retried
> without BTF.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
>  tools/lib/bpf/libbpf.c | 50 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2a82f49ce16f..56a905b502c9 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -998,6 +998,31 @@ find_struct_ops_kern_types(const struct btf *btf, const char *tname,
>         return 0;
>  }
>
> +/* Should match alloc_obj_fields in kernel/bpf/btf.c
> + */

nit: keep comment on a single line?

> +static const char *alloc_obj_fields[] = {
> +       "bpf_spin_lock",
> +       "bpf_list_head",
> +       "bpf_list_node",
> +       "bpf_rb_root",
> +       "bpf_rb_node",
> +};
> +
> +static bool
> +btf_has_alloc_obj_type(const struct btf *btf)

I find "alloc_obj_type" naming completely unhelpful, tbh. Let's use
something more generic and unassuming as "special_btf_type" or
something along those lines?

> +{
> +       const char *tname;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(alloc_obj_fields); i++) {
> +               tname = alloc_obj_fields[i];
> +               if (btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT) > 0)

this will do linear search over entire program's BTF for each
alloc_obj_fields element. Given alloc_obj_fields is supposed to be a
small array, I think it's better to do single linear pass over prog
BTF and for each found STRUCT check if its name matches
alloc_obj_fields.

Having said that, it feels like the better logic would be to check
that any map value's BTF (including global var ARRAYs) have a field of
one of those special types. Just searching for any STRUCT type with
one of those names feels off.

> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  static bool bpf_map__is_struct_ops(const struct bpf_map *map)
>  {
>         return map->def.type == BPF_MAP_TYPE_STRUCT_OPS;
> @@ -2794,7 +2819,8 @@ static bool libbpf_needs_btf(const struct bpf_object *obj)
>
>  static bool kernel_needs_btf(const struct bpf_object *obj)
>  {
> -       return obj->efile.st_ops_shndx >= 0;
> +       return obj->efile.st_ops_shndx >= 0 ||
> +               (obj->btf && btf_has_alloc_obj_type(obj->btf));
>  }
>
>  static int bpf_object__init_btf(struct bpf_object *obj,
> @@ -5103,16 +5129,18 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>
>                 err = -errno;
>                 cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> -               pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
> -                       map->name, cp, err);
> -               create_attr.btf_fd = 0;
> -               create_attr.btf_key_type_id = 0;
> -               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);
> +               pr_warn("Error in bpf_create_map_xattr(%s):%s(%d).\n", map->name, cp, err);
> +               if (!kernel_needs_btf(obj)) {

see above about check whether a map's value BTF itself is using any of
the special type. I think this decision should be made based on
particular map's need for BTF, not based on kernel_needs_btf().

I think it would be better to have an if/else with different
pr_warn()s. Both should report that initial bpf_map_create() (btw,
gotta update the message now, missed that) failed with error, but then
in one case say that we are retrying without BTF, and in another
explain that we are not because map requires kernel to see its BTF.
WDYT?

> +                       pr_warn("Retrying bpf_map_create_xattr(%s) without BTF.\n", map->name);
> +                       create_attr.btf_fd = 0;
> +                       create_attr.btf_key_type_id = 0;
> +                       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);
> +               }
>         }
>
>         err = map->fd < 0 ? -errno : 0;
> --
> 2.30.2
>