[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b9e91dbb-03df-e7d2-8fd9-25bbc77c5188@fb.com>
Date: Mon, 11 Jan 2021 17:34:30 -0800
From: Yonghong Song <yhs@...com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
CC: Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
Networking <netdev@...r.kernel.org>,
Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>,
Kernel Team <kernel-team@...com>, Hao Luo <haoluo@...gle.com>
Subject: Re: [PATCH v2 bpf-next 6/7] libbpf: support kernel module ksym
externs
On 1/11/21 1:37 PM, Andrii Nakryiko wrote:
> On Sun, Jan 10, 2021 at 8:15 PM Yonghong Song <yhs@...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@...nel.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);
>>> }
>>>
Powered by blists - more mailing lists