| Message ID | 20200903223332.881541-3-haoluo@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: BTF support for ksyms | expand |
On Thu, Sep 3, 2020 at 3:34 PM Hao Luo <haoluo@google.com> wrote: > > If a ksym is defined with a type, libbpf will try to find the ksym's btf > information from kernel btf. If a valid btf entry for the ksym is found, > libbpf can pass in the found btf id to the verifier, which validates the > ksym's type and value. > > Typeless ksyms (i.e. those defined as 'void') will not have such btf_id, > but it has the symbol's address (read from kallsyms) and its value is > treated as a raw pointer. > > Signed-off-by: Hao Luo <haoluo@google.com> > --- Logic looks correct, but I have complaints about libbpf logging consistency, please see suggestions below. > tools/lib/bpf/libbpf.c | 116 ++++++++++++++++++++++++++++++++++++----- > 1 file changed, 102 insertions(+), 14 deletions(-) > [...] > @@ -3119,6 +3130,8 @@ static int bpf_object__collect_externs(struct bpf_object *obj) > vt->type = int_btf_id; > vs->offset = off; > vs->size = sizeof(int); > + pr_debug("ksym var_secinfo: var '%s', type #%d, size %d, offset %d\n", > + ext->name, vt->type, vs->size, vs->offset); debug leftover? > } > sec->size = off; > } > @@ -5724,8 +5737,13 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj) > insn[0].imm = obj->maps[obj->kconfig_map_idx].fd; > insn[1].imm = ext->kcfg.data_off; > } else /* EXT_KSYM */ { > - insn[0].imm = (__u32)ext->ksym.addr; > - insn[1].imm = ext->ksym.addr >> 32; > + if (ext->ksym.type_id) { /* typed ksyms */ > + insn[0].src_reg = BPF_PSEUDO_BTF_ID; > + insn[0].imm = ext->ksym.vmlinux_btf_id; > + } else { /* typeless ksyms */ > + insn[0].imm = (__u32)ext->ksym.addr; > + insn[1].imm = ext->ksym.addr >> 32; > + } > } > break; > case RELO_CALL: > @@ -6462,10 +6480,72 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj) > return err; > } > > +static int bpf_object__resolve_ksyms_btf_id(struct bpf_object *obj) > +{ > + struct extern_desc *ext; > + int i, id; > + > + if (!obj->btf_vmlinux) { > + pr_warn("support of typed ksyms needs kernel btf.\n"); > + return -ENOENT; > + } This check shouldn't be needed, you'd either successfully load btf_vmlinux by now or will fail earlier, because BTF is required but not found. > + > + for (i = 0; i < obj->nr_extern; i++) { > + const struct btf_type *targ_var, *targ_type; > + __u32 targ_type_id, local_type_id; > + int ret; > + > + ext = &obj->externs[i]; > + if (ext->type != EXT_KSYM || !ext->ksym.type_id) > + continue; > + > + id = btf__find_by_name_kind(obj->btf_vmlinux, ext->name, > + BTF_KIND_VAR); > + if (id <= 0) { > + pr_warn("no btf entry for ksym '%s' in vmlinux.\n", > + ext->name); please try to stick to consistent style of comments: "extern (ksym) '%s': failed to find BTF ID in vmlinux BTF" or something like that > + return -ESRCH; > + } > + > + /* find target type_id */ > + targ_var = btf__type_by_id(obj->btf_vmlinux, id); > + targ_type = skip_mods_and_typedefs(obj->btf_vmlinux, > + targ_var->type, > + &targ_type_id); > + > + /* find local type_id */ > + local_type_id = ext->ksym.type_id; > + > + ret = bpf_core_types_are_compat(obj->btf_vmlinux, targ_type_id, > + obj->btf, local_type_id); you reversed the order, it's always local btf/id, then target btf/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); > + 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); it's a bit unfortunate that we get the name of an already resolved type, because if you have a typedef to anon struct, this will give you an empty string. I don't know how much of a problem that would be, so I think it's fine to leave it as is, and fix it if it's a problem in practice. > + > + pr_warn("ksym '%s' expects type '%s' (vmlinux_btf_id: #%d), " > + "but got '%s' (btf_id: #%d)\n", ext->name, > + targ_name, targ_type_id, local_name, local_type_id); same thing, please stay consistent in logging format. Check bpf_core_dump_spec() for how BTF type info is usually emitted throughout libbpf: "extern (ksym): incompatible types, expected [%d] %s %s, but kernel has [%d] %s %s\n" there is a btf_kind_str() helper to resolve kind to a string representation. > + return -EINVAL; > + } > + > + ext->is_set = true; > + ext->ksym.vmlinux_btf_id = id; > + pr_debug("extern (ksym) %s=vmlinux_btf_id(#%d)\n", ext->name, id); "extern (ksym) '%s': resolved to [%d] %s %s\n", similar to above suggestion. This "[%d]" format is very consistently used for BTF IDs throughout, so it will be familiar and recognizable for people that had to deal with this in libbpf logs. > + } > + return 0; > +} > + > static int bpf_object__resolve_externs(struct bpf_object *obj, > const char *extra_kconfig) > { > - bool need_config = false, need_kallsyms = false; > + bool need_kallsyms = false, need_vmlinux_btf = false; > + bool need_config = false; nit: doesn't make sense to change the existing source code line at all. Just add `bool need_vmlinux_btf = false;` on a new line? Or we can split all these bools into 3 separate lines, if you prefer. > struct extern_desc *ext; > void *kcfg_data = NULL; > int err, i; [...]
Will follow the libbpf logging convention. Thanks for the suggestions. On Fri, Sep 4, 2020 at 12:34 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Sep 3, 2020 at 3:34 PM Hao Luo <haoluo@google.com> wrote: > > > > If a ksym is defined with a type, libbpf will try to find the ksym's btf > > information from kernel btf. If a valid btf entry for the ksym is found, > > libbpf can pass in the found btf id to the verifier, which validates the > > ksym's type and value. > > > > Typeless ksyms (i.e. those defined as 'void') will not have such btf_id, > > but it has the symbol's address (read from kallsyms) and its value is > > treated as a raw pointer. > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > --- > > Logic looks correct, but I have complaints about libbpf logging > consistency, please see suggestions below. > > > tools/lib/bpf/libbpf.c | 116 ++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 102 insertions(+), 14 deletions(-) > > > > [...] > > > @@ -3119,6 +3130,8 @@ static int bpf_object__collect_externs(struct bpf_object *obj) > > vt->type = int_btf_id; > > vs->offset = off; > > vs->size = sizeof(int); > > + pr_debug("ksym var_secinfo: var '%s', type #%d, size %d, offset %d\n", > > + ext->name, vt->type, vs->size, vs->offset); > > debug leftover? > I was thinking we should leave a debug message when some entries in BTF are modified. It's probably unnecessary, as I'm thinking of it right now. I will remove this in v3. > > } > > sec->size = off; > > } > > @@ -5724,8 +5737,13 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj) > > insn[0].imm = obj->maps[obj->kconfig_map_idx].fd; > > insn[1].imm = ext->kcfg.data_off; > > } else /* EXT_KSYM */ { > > - insn[0].imm = (__u32)ext->ksym.addr; > > - insn[1].imm = ext->ksym.addr >> 32; > > + if (ext->ksym.type_id) { /* typed ksyms */ > > + insn[0].src_reg = BPF_PSEUDO_BTF_ID; > > + insn[0].imm = ext->ksym.vmlinux_btf_id; > > + } else { /* typeless ksyms */ > > + insn[0].imm = (__u32)ext->ksym.addr; > > + insn[1].imm = ext->ksym.addr >> 32; > > + } > > } > > break; > > case RELO_CALL: > > @@ -6462,10 +6480,72 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj) > > return err; > > } > > > > +static int bpf_object__resolve_ksyms_btf_id(struct bpf_object *obj) > > +{ > > + struct extern_desc *ext; > > + int i, id; > > + > > + if (!obj->btf_vmlinux) { > > + pr_warn("support of typed ksyms needs kernel btf.\n"); > > + return -ENOENT; > > + } > > This check shouldn't be needed, you'd either successfully load > btf_vmlinux by now or will fail earlier, because BTF is required but > not found. > > > + > > + for (i = 0; i < obj->nr_extern; i++) { > > + const struct btf_type *targ_var, *targ_type; > > + __u32 targ_type_id, local_type_id; > > + int ret; > > + > > + ext = &obj->externs[i]; > > + if (ext->type != EXT_KSYM || !ext->ksym.type_id) > > + continue; > > + > > + id = btf__find_by_name_kind(obj->btf_vmlinux, ext->name, > > + BTF_KIND_VAR); > > + if (id <= 0) { > > + pr_warn("no btf entry for ksym '%s' in vmlinux.\n", > > + ext->name); > > please try to stick to consistent style of comments: > > "extern (ksym) '%s': failed to find BTF ID in vmlinux BTF" or > something like that > > > > + return -ESRCH; > > + } > > + > > + /* find target type_id */ > > + targ_var = btf__type_by_id(obj->btf_vmlinux, id); > > + targ_type = skip_mods_and_typedefs(obj->btf_vmlinux, > > + targ_var->type, > > + &targ_type_id); > > + > > + /* find local type_id */ > > + local_type_id = ext->ksym.type_id; > > + > > + ret = bpf_core_types_are_compat(obj->btf_vmlinux, targ_type_id, > > + obj->btf, local_type_id); > > you reversed the order, it's always local btf/id, then target btf/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); > > + 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); > > it's a bit unfortunate that we get the name of an already resolved > type, because if you have a typedef to anon struct, this will give you > an empty string. I don't know how much of a problem that would be, so > I think it's fine to leave it as is, and fix it if it's a problem in > practice. > > > + > > + pr_warn("ksym '%s' expects type '%s' (vmlinux_btf_id: #%d), " > > + "but got '%s' (btf_id: #%d)\n", ext->name, > > + targ_name, targ_type_id, local_name, local_type_id); > > same thing, please stay consistent in logging format. Check > bpf_core_dump_spec() for how BTF type info is usually emitted > throughout libbpf: > > "extern (ksym): incompatible types, expected [%d] %s %s, but kernel > has [%d] %s %s\n" > > there is a btf_kind_str() helper to resolve kind to a string representation. > > > > + return -EINVAL; > > + } > > + > > + ext->is_set = true; > > + ext->ksym.vmlinux_btf_id = id; > > + pr_debug("extern (ksym) %s=vmlinux_btf_id(#%d)\n", ext->name, id); > > "extern (ksym) '%s': resolved to [%d] %s %s\n", similar to above > suggestion. This "[%d]" format is very consistently used for BTF IDs > throughout, so it will be familiar and recognizable for people that > had to deal with this in libbpf logs. > > > + } > > + return 0; > > +} > > + > > static int bpf_object__resolve_externs(struct bpf_object *obj, > > const char *extra_kconfig) > > { > > - bool need_config = false, need_kallsyms = false; > > + bool need_kallsyms = false, need_vmlinux_btf = false; > > + bool need_config = false; > > nit: doesn't make sense to change the existing source code line at > all. Just add `bool need_vmlinux_btf = false;` on a new line? Or we > can split all these bools into 3 separate lines, if you prefer. > > > struct extern_desc *ext; > > void *kcfg_data = NULL; > > int err, i; > > [...]
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index b688aadf09c5..c7a8d7d72b46 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -361,6 +361,12 @@ struct extern_desc { } kcfg; struct { unsigned long long addr; + + /* target btf_id of the corresponding kernel var. */ + int vmlinux_btf_id; + + /* local btf_id of the ksym extern's type. */ + __u32 type_id; } ksym; }; }; @@ -2479,12 +2485,23 @@ static int bpf_object__load_vmlinux_btf(struct bpf_object *obj) { bool need_vmlinux_btf = false; struct bpf_program *prog; - int err; + int i, err; /* CO-RE relocations need kernel BTF */ if (obj->btf_ext && obj->btf_ext->core_relo_info.len) need_vmlinux_btf = true; + /* Support for typed ksyms needs kernel BTF */ + for (i = 0; i < obj->nr_extern; i++) { + const struct extern_desc *ext; + + ext = &obj->externs[i]; + if (ext->type == EXT_KSYM && ext->ksym.type_id) { + need_vmlinux_btf = true; + break; + } + } + bpf_object__for_each_program(prog, obj) { if (!prog->load) continue; @@ -3060,16 +3077,10 @@ static int bpf_object__collect_externs(struct bpf_object *obj) return -ENOTSUP; } } else if (strcmp(sec_name, KSYMS_SEC) == 0) { - const struct btf_type *vt; - ksym_sec = sec; ext->type = EXT_KSYM; - - vt = skip_mods_and_typedefs(obj->btf, t->type, NULL); - if (!btf_is_void(vt)) { - pr_warn("extern (ksym) '%s' is not typeless (void)\n", ext_name); - return -ENOTSUP; - } + skip_mods_and_typedefs(obj->btf, t->type, + &ext->ksym.type_id); } else { pr_warn("unrecognized extern section '%s'\n", sec_name); return -ENOTSUP; @@ -3119,6 +3130,8 @@ static int bpf_object__collect_externs(struct bpf_object *obj) vt->type = int_btf_id; vs->offset = off; vs->size = sizeof(int); + pr_debug("ksym var_secinfo: var '%s', type #%d, size %d, offset %d\n", + ext->name, vt->type, vs->size, vs->offset); } sec->size = off; } @@ -5724,8 +5737,13 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj) insn[0].imm = obj->maps[obj->kconfig_map_idx].fd; insn[1].imm = ext->kcfg.data_off; } else /* EXT_KSYM */ { - insn[0].imm = (__u32)ext->ksym.addr; - insn[1].imm = ext->ksym.addr >> 32; + if (ext->ksym.type_id) { /* typed ksyms */ + insn[0].src_reg = BPF_PSEUDO_BTF_ID; + insn[0].imm = ext->ksym.vmlinux_btf_id; + } else { /* typeless ksyms */ + insn[0].imm = (__u32)ext->ksym.addr; + insn[1].imm = ext->ksym.addr >> 32; + } } break; case RELO_CALL: @@ -6462,10 +6480,72 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj) return err; } +static int bpf_object__resolve_ksyms_btf_id(struct bpf_object *obj) +{ + struct extern_desc *ext; + int i, id; + + if (!obj->btf_vmlinux) { + pr_warn("support of typed ksyms needs kernel btf.\n"); + return -ENOENT; + } + + for (i = 0; i < obj->nr_extern; i++) { + const struct btf_type *targ_var, *targ_type; + __u32 targ_type_id, local_type_id; + int ret; + + ext = &obj->externs[i]; + if (ext->type != EXT_KSYM || !ext->ksym.type_id) + continue; + + id = btf__find_by_name_kind(obj->btf_vmlinux, ext->name, + BTF_KIND_VAR); + if (id <= 0) { + pr_warn("no btf entry for ksym '%s' in vmlinux.\n", + ext->name); + return -ESRCH; + } + + /* find target type_id */ + targ_var = btf__type_by_id(obj->btf_vmlinux, id); + targ_type = skip_mods_and_typedefs(obj->btf_vmlinux, + targ_var->type, + &targ_type_id); + + /* find local type_id */ + local_type_id = ext->ksym.type_id; + + ret = bpf_core_types_are_compat(obj->btf_vmlinux, targ_type_id, + obj->btf, local_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); + 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); + + pr_warn("ksym '%s' expects type '%s' (vmlinux_btf_id: #%d), " + "but got '%s' (btf_id: #%d)\n", ext->name, + targ_name, targ_type_id, local_name, local_type_id); + return -EINVAL; + } + + ext->is_set = true; + ext->ksym.vmlinux_btf_id = id; + pr_debug("extern (ksym) %s=vmlinux_btf_id(#%d)\n", ext->name, id); + } + return 0; +} + static int bpf_object__resolve_externs(struct bpf_object *obj, const char *extra_kconfig) { - bool need_config = false, need_kallsyms = false; + bool need_kallsyms = false, need_vmlinux_btf = false; + bool need_config = false; struct extern_desc *ext; void *kcfg_data = NULL; int err, i; @@ -6496,7 +6576,10 @@ static int bpf_object__resolve_externs(struct bpf_object *obj, strncmp(ext->name, "CONFIG_", 7) == 0) { need_config = true; } else if (ext->type == EXT_KSYM) { - need_kallsyms = true; + if (ext->ksym.type_id) + need_vmlinux_btf = true; + else + need_kallsyms = true; } else { pr_warn("unrecognized extern '%s'\n", ext->name); return -EINVAL; @@ -6525,6 +6608,11 @@ static int bpf_object__resolve_externs(struct bpf_object *obj, if (err) return -EINVAL; } + if (need_vmlinux_btf) { + err = bpf_object__resolve_ksyms_btf_id(obj); + if (err) + return -EINVAL; + } for (i = 0; i < obj->nr_extern; i++) { ext = &obj->externs[i]; @@ -6557,10 +6645,10 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) } err = bpf_object__probe_loading(obj); + err = err ? : bpf_object__load_vmlinux_btf(obj); err = err ? : bpf_object__resolve_externs(obj, obj->kconfig); err = err ? : bpf_object__sanitize_and_load_btf(obj); err = err ? : bpf_object__sanitize_maps(obj); - err = err ? : bpf_object__load_vmlinux_btf(obj); err = err ? : bpf_object__init_kern_struct_ops_maps(obj); err = err ? : bpf_object__create_maps(obj); err = err ? : bpf_object__relocate(obj, attr->target_btf_path);
If a ksym is defined with a type, libbpf will try to find the ksym's btf information from kernel btf. If a valid btf entry for the ksym is found, libbpf can pass in the found btf id to the verifier, which validates the ksym's type and value. Typeless ksyms (i.e. those defined as 'void') will not have such btf_id, but it has the symbol's address (read from kallsyms) and its value is treated as a raw pointer. Signed-off-by: Hao Luo <haoluo@google.com> --- tools/lib/bpf/libbpf.c | 116 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 102 insertions(+), 14 deletions(-)