On Tue, Oct 17, 2023 at 9:23 AM <thinker.li@xxxxxxxxx> wrote:
>
> From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
>
> Locate the module BTFs for struct_ops maps and progs and pass them to the
> kernel. This ensures that the kernel correctly resolves type IDs from the
> appropriate 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. The FD of the module BTF is passed to the
> kernel as mod_btf_fd when the struct_ops object is loaded.
>
> For a bpf_struct_ops prog, attach_btf_obj_fd of bpf_prog is the FD of a
> module BTF in the kernel.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> ---
> tools/lib/bpf/bpf.c | 4 ++-
> tools/lib/bpf/bpf.h | 5 +++-
> tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++---------------
> 3 files changed, 51 insertions(+), 26 deletions(-)
>
I have a few nits, please accommodate them, and with that please add
my ack on libbpf side of things
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index b0f1913763a3..af46488e4ea9 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -169,7 +169,8 @@ 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,
> + value_type_btf_obj_fd);
> union bpf_attr attr;
> int fd;
>
> @@ -191,6 +192,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.value_type_btf_obj_fd = OPTS_GET(opts, value_type_btf_obj_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..1733cdc21241 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -51,8 +51,11 @@ struct bpf_map_create_opts {
>
> __u32 numa_node;
> __u32 map_ifindex;
> +
> + __u32 value_type_btf_obj_fd;
> + size_t:0;
> };
> -#define bpf_map_create_opts__last_field map_ifindex
> +#define bpf_map_create_opts__last_field value_type_btf_obj_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..d8a60fb52f5c 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;
> + int mod_btf_fd;
> __u32 btf_key_type_id;
> __u32 btf_value_type_id;
> __u32 btf_vmlinux_value_type_id;
> @@ -893,6 +894,8 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
> return 0;
> }
>
> +static int load_module_btfs(struct bpf_object *obj);
> +
you don't need this forward declaration, do you?
> static const struct btf_member *
> find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
> {
> @@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
> return NULL;
> }
>
[...]
> if (obj->btf && btf__fd(obj->btf) >= 0) {
> create_attr.btf_fd = btf__fd(obj->btf);
> @@ -7700,9 +7718,9 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
> return libbpf_kallsyms_parse(kallsyms_cb, obj);
> }
>
> -static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
> - __u16 kind, struct btf **res_btf,
> - struct module_btf **res_mod_btf)
> +static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
> + __u16 kind, struct btf **res_btf,
> + struct module_btf **res_mod_btf)
I actually find "find_module" terminology confusing, because it might
not be in the module after all, right? I think "find_ksym_btf_id" is a
totally appropriate name, and it's just that in some cases that kernel
symbol (ksym) will be found in the kernel module instead of in vmlinux
image itself. Still, it's a kernel. Let's keep the name?
> {
> struct module_btf *mod_btf;
> struct btf *btf;
[...]
