Re: [PATCH v2 bpf-next 6/7] libbpf: support kernel module ksym externs

[Yonghong Song <yhs@fb.com>]

On 1/11/21 1:37 PM, Andrii Nakryiko wrote:
> On Sun, Jan 10, 2021 at 8:15 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 1/8/21 2:09 PM, Andrii Nakryiko wrote:
>>> Add support for searching for ksym externs not just in vmlinux BTF, but across
>>> all module BTFs, similarly to how it's done for CO-RE relocations. Kernels
>>> that expose module BTFs through sysfs are assumed to support new ldimm64
>>> instruction extension with BTF FD provided in insn[1].imm field, so no extra
>>> feature detection is performed.
>>>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>> ---
>>>    tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++---------------
>>>    1 file changed, 30 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>> index 6ae748f6ea11..57559a71e4de 100644
>>> --- a/tools/lib/bpf/libbpf.c
>>> +++ b/tools/lib/bpf/libbpf.c
>>> @@ -395,7 +395,8 @@ struct extern_desc {
>>>                        unsigned long long addr;
>>>
>>>                        /* target btf_id of the corresponding kernel var. */
>>> -                     int vmlinux_btf_id;
>>> +                     int kernel_btf_obj_fd;
>>> +                     int kernel_btf_id;
>>>
>>>                        /* local btf_id of the ksym extern's type. */
>>>                        __u32 type_id;
>>> @@ -6162,7 +6163,8 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
>>>                        } else /* EXT_KSYM */ {
>>>                                if (ext->ksym.type_id) { /* typed ksyms */
>>>                                        insn[0].src_reg = BPF_PSEUDO_BTF_ID;
>>> -                                     insn[0].imm = ext->ksym.vmlinux_btf_id;
>>> +                                     insn[0].imm = ext->ksym.kernel_btf_id;
>>> +                                     insn[1].imm = ext->ksym.kernel_btf_obj_fd;
>>>                                } else { /* typeless ksyms */
>>>                                        insn[0].imm = (__u32)ext->ksym.addr;
>>>                                        insn[1].imm = ext->ksym.addr >> 32;
>>> @@ -7319,7 +7321,8 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
>>>    static int bpf_object__resolve_ksyms_btf_id(struct bpf_object *obj)
>>>    {
>>>        struct extern_desc *ext;
>>> -     int i, id;
>>> +     struct btf *btf;
>>> +     int i, j, id, btf_fd, err;
>>>
>>>        for (i = 0; i < obj->nr_extern; i++) {
>>>                const struct btf_type *targ_var, *targ_type;
>>> @@ -7331,8 +7334,22 @@ static int bpf_object__resolve_ksyms_btf_id(struct bpf_object *obj)
>>>                if (ext->type != EXT_KSYM || !ext->ksym.type_id)
>>>                        continue;
>>>
>>> -             id = btf__find_by_name_kind(obj->btf_vmlinux, ext->name,
>>> -                                         BTF_KIND_VAR);
>>> +             btf = obj->btf_vmlinux;
>>> +             btf_fd = 0;
>>> +             id = btf__find_by_name_kind(btf, ext->name, BTF_KIND_VAR);
>>> +             if (id == -ENOENT) {
>>> +                     err = load_module_btfs(obj);
>>> +                     if (err)
>>> +                             return err;
>>> +
>>> +                     for (j = 0; j < obj->btf_module_cnt; j++) {
>>> +                             btf = obj->btf_modules[j].btf;
>>> +                             btf_fd = obj->btf_modules[j].fd;
>>
>> Do we have possibility btf_fd == 0 here?
> 
> Extremely unlikely. But if we are really worried about 0 fd, we should
> handle that in a centralized fashion in libbpf. I.e., for any
> operation that can return FD, check if that FD is 0, and if yes, dup()
> it. And then make everything call that helper. So in the context of
> this patch I'm just ignoring such possibility.
Maybe at least add some comments here to document such a possibility?

> 
>>
>>> +                             id = btf__find_by_name_kind(btf, ext->name, BTF_KIND_VAR);
>>> +                             if (id != -ENOENT)
>>> +                                     break;
>>> +                     }
>>> +             }
>>>                if (id <= 0) {
>>>                        pr_warn("extern (ksym) '%s': failed to find BTF ID in vmlinux BTF.\n",
>>>                                ext->name);
>>> @@ -7343,24 +7360,19 @@ static int bpf_object__resolve_ksyms_btf_id(struct bpf_object *obj)
>>>                local_type_id = ext->ksym.type_id;
>>>
>>>                /* find target type_id */
>>> -             targ_var = btf__type_by_id(obj->btf_vmlinux, id);
>>> -             targ_var_name = btf__name_by_offset(obj->btf_vmlinux,
>>> -                                                 targ_var->name_off);
>>> -             targ_type = skip_mods_and_typedefs(obj->btf_vmlinux,
>>> -                                                targ_var->type,
>>> -                                                &targ_type_id);
>>> +             targ_var = btf__type_by_id(btf, id);
>>> +             targ_var_name = btf__name_by_offset(btf, targ_var->name_off);
>>> +             targ_type = skip_mods_and_typedefs(btf, targ_var->type, &targ_type_id);
>>>
>>>                ret = bpf_core_types_are_compat(obj->btf, local_type_id,
>>> -                                             obj->btf_vmlinux, targ_type_id);
>>> +                                             btf, targ_type_id);
>>>                if (ret <= 0) {
>>>                        const struct btf_type *local_type;
>>>                        const char *targ_name, *local_name;
>>>
>>>                        local_type = btf__type_by_id(obj->btf, local_type_id);
>>> -                     local_name = btf__name_by_offset(obj->btf,
>>> -                                                      local_type->name_off);
>>> -                     targ_name = btf__name_by_offset(obj->btf_vmlinux,
>>> -                                                     targ_type->name_off);
>>> +                     local_name = btf__name_by_offset(obj->btf, local_type->name_off);
>>> +                     targ_name = btf__name_by_offset(btf, targ_type->name_off);
>>>
>>>                        pr_warn("extern (ksym) '%s': incompatible types, expected [%d] %s %s, but kernel has [%d] %s %s\n",
>>>                                ext->name, local_type_id,
>>> @@ -7370,7 +7382,8 @@ static int bpf_object__resolve_ksyms_btf_id(struct bpf_object *obj)
>>>                }
>>>
>>>                ext->is_set = true;
>>> -             ext->ksym.vmlinux_btf_id = id;
>>> +             ext->ksym.kernel_btf_obj_fd = btf_fd;
>>> +             ext->ksym.kernel_btf_id = id;
>>>                pr_debug("extern (ksym) '%s': resolved to [%d] %s %s\n",
>>>                         ext->name, id, btf_kind_str(targ_var), targ_var_name);
>>>        }
>>>