On Wed, Sep 20, 2023 at 9:00 AM <thinker.li@xxxxxxxxx> wrote:
>
> From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
>
> Find module BTFs for struct_ops maps and progs and pass them to the kernel.
> It ensures the kernel resolve type IDs from correct module BTFs.
>
> For the map of a struct_ops object, mod_btf is added to bpf_map to keep a
> reference to the module BTF. It's FD is passed to the kernel as mod_btf_fd
> when it is created.
>
> For a prog attaching to a struct_ops object, attach_btf_obj_fd of bpf_prog
> is the FD pointing to a module BTF in the kernel.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> ---
> tools/lib/bpf/bpf.c | 3 +-
> tools/lib/bpf/bpf.h | 4 +-
> tools/lib/bpf/libbpf.c | 121 ++++++++++++++++++++++++-----------------
> 3 files changed, 75 insertions(+), 53 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index b0f1913763a3..df4b7570ad92 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -169,7 +169,7 @@ int bpf_map_create(enum bpf_map_type map_type,
> __u32 max_entries,
> const struct bpf_map_create_opts *opts)
> {
> - const size_t attr_sz = offsetofend(union bpf_attr, map_extra);
> + const size_t attr_sz = offsetofend(union bpf_attr, mod_btf_fd);
> union bpf_attr attr;
> int fd;
>
> @@ -191,6 +191,7 @@ int bpf_map_create(enum bpf_map_type map_type,
> attr.btf_key_type_id = OPTS_GET(opts, btf_key_type_id, 0);
> attr.btf_value_type_id = OPTS_GET(opts, btf_value_type_id, 0);
> attr.btf_vmlinux_value_type_id = OPTS_GET(opts, btf_vmlinux_value_type_id, 0);
> + attr.mod_btf_fd = OPTS_GET(opts, mod_btf_fd, 0);
>
> attr.inner_map_fd = OPTS_GET(opts, inner_map_fd, 0);
> attr.map_flags = OPTS_GET(opts, map_flags, 0);
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 74c2887cfd24..d18f75b0ccc9 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -51,8 +51,10 @@ struct bpf_map_create_opts {
>
> __u32 numa_node;
> __u32 map_ifindex;
> +
> + __u32 mod_btf_fd;
please add `size_t :0;` at the end to avoid compiler leaving garbage
in added padding at the end of opts struct
> };
> -#define bpf_map_create_opts__last_field map_ifindex
> +#define bpf_map_create_opts__last_field mod_btf_fd
>
> LIBBPF_API int bpf_map_create(enum bpf_map_type map_type,
> const char *map_name,
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3a6108e3238b..df6ba5494adb 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -519,6 +519,7 @@ struct bpf_map {
> struct bpf_map_def def;
> __u32 numa_node;
> __u32 btf_var_idx;
> + struct module_btf *mod_btf;
It would be simpler to just store btf_fd instead of entire struct
module_btf pointer. You only need this to set btf_obj_fd on map
creation and program loading, right?
> __u32 btf_key_type_id;
> __u32 btf_value_type_id;
> __u32 btf_vmlinux_value_type_id;
> @@ -893,6 +894,42 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
> return 0;
> }
>
> +static int load_module_btfs(struct bpf_object *obj);
> +
> +static int find_kern_btf_id(struct bpf_object *obj, const char *kern_name,
> + __u16 kind, struct btf **res_btf,
> + struct module_btf **res_mod_btf)
> +{
> + struct module_btf *mod_btf;
> + struct btf *btf;
> + int i, id, err;
> +
> + btf = obj->btf_vmlinux;
> + mod_btf = NULL;
> + id = btf__find_by_name_kind(btf, kern_name, kind);
> +
> + if (id == -ENOENT) {
> + err = load_module_btfs(obj);
> + if (err)
> + return err;
> +
> + for (i = 0; i < obj->btf_module_cnt; i++) {
> + /* we assume module_btf's BTF FD is always >0 */
> + mod_btf = &obj->btf_modules[i];
> + btf = mod_btf->btf;
> + id = btf__find_by_name_kind_own(btf, kern_name, kind);
> + if (id != -ENOENT)
> + break;
> + }
> + }
> + if (id <= 0)
> + return -ESRCH;
> +
> + *res_btf = btf;
> + *res_mod_btf = mod_btf;
> + return id;
> +}
> +
there is no need to move the entire function body here, just add a
forward declaration. It will also make it easier to see what actually
changed about the function (if at all)
> static const struct btf_member *
> find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
> {
> @@ -927,17 +964,23 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
> const char *name, __u32 kind);
>
> static int
> -find_struct_ops_kern_types(const struct btf *btf, const char *tname,
> +find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> + struct module_btf **mod_btf,
> const struct btf_type **type, __u32 *type_id,
> const struct btf_type **vtype, __u32 *vtype_id,
> const struct btf_member **data_member)
> {
> const struct btf_type *kern_type, *kern_vtype;
> const struct btf_member *kern_data_member;
> + struct btf *btf;
> __s32 kern_vtype_id, kern_type_id;
> __u32 i;
>
> - kern_type_id = btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT);
> + /* XXX: should search module BTFs as well. We need module name here
> + * to locate a correct BTF type.
> + */
aren't we searching module BTFs? Is this comment still relevant?
> + kern_type_id = find_kern_btf_id(obj, tname, BTF_KIND_STRUCT,
> + &btf, mod_btf);
> if (kern_type_id < 0) {
> pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
> tname);
> @@ -992,13 +1035,15 @@ static bool bpf_map__is_struct_ops(const struct bpf_map *map)
>
> /* Init the map's fields that depend on kern_btf */
> static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
> - const struct btf *btf,
> - const struct btf *kern_btf)
> + struct bpf_object *obj)
no need to pass obj separately, you can get it through `map->obj`
> {
> const struct btf_member *member, *kern_member, *kern_data_member;
> const struct btf_type *type, *kern_type, *kern_vtype;
> __u32 i, kern_type_id, kern_vtype_id, kern_data_off;
> struct bpf_struct_ops *st_ops;
> + const struct btf *kern_btf;
> + struct module_btf *mod_btf;
> + const struct btf *btf = obj->btf;
> void *data, *kern_data;
> const char *tname;
> int err;
> @@ -1006,16 +1051,19 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
> st_ops = map->st_ops;
> type = st_ops->type;
> tname = st_ops->tname;
> - err = find_struct_ops_kern_types(kern_btf, tname,
> + err = find_struct_ops_kern_types(obj, tname, &mod_btf,
> &kern_type, &kern_type_id,
> &kern_vtype, &kern_vtype_id,
> &kern_data_member);
> if (err)
> return err;
>
> + kern_btf = mod_btf ? mod_btf->btf : obj->btf_vmlinux;
> +
> pr_debug("struct_ops init_kern %s: type_id:%u kern_type_id:%u kern_vtype_id:%u\n",
> map->name, st_ops->type_id, kern_type_id, kern_vtype_id);
>
> + map->mod_btf = mod_btf;
> map->def.value_size = kern_vtype->size;
> map->btf_vmlinux_value_type_id = kern_vtype_id;
>
> @@ -1091,6 +1139,9 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
> return -ENOTSUP;
> }
>
> + /* XXX: attach_btf_obj_fd is needed as well */
seems like all these XXX comments are outdated and the code is already
doing all that, is that right? If so, please remove them, they are
confusing
> + if (mod_btf)
> + prog->attach_btf_obj_fd = mod_btf->fd;
> prog->attach_btf_id = kern_type_id;
> prog->expected_attach_type = kern_member_idx;
>
> @@ -1133,8 +1184,8 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
> if (!bpf_map__is_struct_ops(map))
> continue;
>
> - err = bpf_map__init_kern_struct_ops(map, obj->btf,
> - obj->btf_vmlinux);
> + /* XXX: should be a module btf if not vmlinux */
> + err = bpf_map__init_kern_struct_ops(map, obj);
> if (err)
> return err;
> }
[...]
