On Tue, Mar 16, 2021 at 12:02 AM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> This patch is to make libbpf able to handle the following extern
> kernel function declaration and do the needed relocations before
> loading the bpf program to the kernel.
>
> extern int foo(struct sock *) __attribute__((section(".ksyms")))
>
> In the collect extern phase, needed changes is made to
> bpf_object__collect_externs() and find_extern_btf_id() to collect
> function.
>
> In the collect relo phase, it will record the kernel function
> call as RELO_EXTERN_FUNC.
>
> bpf_object__resolve_ksym_func_btf_id() is added to find the func
> btf_id of the running kernel.
>
> During actual relocation, it will patch the BPF_CALL instruction with
> src_reg = BPF_PSEUDO_FUNC_CALL and insn->imm set to the running
> kernel func's btf_id.
>
> btf_fixup_datasec() is changed also because a datasec may
> only have func and its size will be 0. The "!size" test
> is postponed till it is confirmed there are vars.
> It also takes this chance to remove the
> "if (... || (t->size && t->size != size)) { return -ENOENT; }" test
> because t->size is zero at the point.
>
> The required LLVM patch: https://reviews.llvm.org/D93563
>
> Signed-off-by: Martin KaFai Lau <kafai@xxxxxx>
> ---
> tools/lib/bpf/btf.c | 32 ++++++++----
> tools/lib/bpf/btf.h | 5 ++
> tools/lib/bpf/libbpf.c | 113 +++++++++++++++++++++++++++++++++++++----
> 3 files changed, 129 insertions(+), 21 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 3aa58f2ac183..bb09b577c154 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -1108,7 +1108,7 @@ static int btf_fixup_datasec(struct bpf_object *obj, struct btf *btf,
> const struct btf_type *t_var;
> struct btf_var_secinfo *vsi;
> const struct btf_var *var;
> - int ret;
> + int ret, nr_vars = 0;
>
> if (!name) {
> pr_debug("No name found in string section for DATASEC kind.\n");
> @@ -1117,27 +1117,27 @@ static int btf_fixup_datasec(struct bpf_object *obj, struct btf *btf,
>
> /* .extern datasec size and var offsets were set correctly during
> * extern collection step, so just skip straight to sorting variables
> + * One exception is the datasec may only have extern funcs,
> + * t->size is 0 in this case. This will be handled
> + * with !nr_vars later.
> */
> if (t->size)
> goto sort_vars;
>
> - ret = bpf_object__section_size(obj, name, &size);
> - if (ret || !size || (t->size && t->size != size)) {
> - pr_debug("Invalid size for section %s: %u bytes\n", name, size);
> - return -ENOENT;
> - }
> -
> - t->size = size;
> + bpf_object__section_size(obj, name, &size);
So it's not great that we just ignore any errors here. ".ksyms" is a
special section, so it should be fine to just ignore it by name and
leave the rest of error handling intact.
>
> for (i = 0, vsi = btf_var_secinfos(t); i < vars; i++, vsi++) {
> t_var = btf__type_by_id(btf, vsi->type);
> - var = btf_var(t_var);
>
> - if (!btf_is_var(t_var)) {
> - pr_debug("Non-VAR type seen in section %s\n", name);
> + if (btf_is_func(t_var)) {
> + continue;
just
if (btf_is_func(t_var))
continue;
no need for "else if" below
> + } else if (!btf_is_var(t_var)) {
> + pr_debug("Non-VAR and Non-FUNC type seen in section %s\n", name);
nit: Non-FUNC -> non-FUNC
> return -EINVAL;
> }
>
> + nr_vars++;
> + var = btf_var(t_var);
> if (var->linkage == BTF_VAR_STATIC)
> continue;
>
> @@ -1157,6 +1157,16 @@ static int btf_fixup_datasec(struct bpf_object *obj, struct btf *btf,
> vsi->offset = off;
> }
>
> + if (!nr_vars)
> + return 0;
> +
> + if (!size) {
> + pr_debug("Invalid size for section %s: %u bytes\n", name, size);
> + return -ENOENT;
> + }
> +
> + t->size = size;
> +
> sort_vars:
> qsort(btf_var_secinfos(t), vars, sizeof(*vsi), compare_vsi_off);
> return 0;
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 029a9cfc8c2d..07d508b70497 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -368,6 +368,11 @@ btf_var_secinfos(const struct btf_type *t)
> return (struct btf_var_secinfo *)(t + 1);
> }
>
> +static inline enum btf_func_linkage btf_func_linkage(const struct btf_type *t)
> +{
> + return (enum btf_func_linkage)BTF_INFO_VLEN(t->info);
> +}
exposing `enum btf_func_linkage` in libbpf API headers will cause
compilation errors for users on older systems. We went through a bunch
of pain with `enum bpf_stats_type` (and it is still causing pain for
C++), I'd rather avoid some more here. Can you please move it into
libbpf.c for now. It doesn't seem like a very popular function that
needs to be exposed to users.
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 0a60fcb2fba2..49bda179bd93 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -190,6 +190,7 @@ enum reloc_type {
> RELO_CALL,
> RELO_DATA,
> RELO_EXTERN_VAR,
> + RELO_EXTERN_FUNC,
> RELO_SUBPROG_ADDR,
> };
>
> @@ -384,6 +385,7 @@ struct extern_desc {
> int btf_id;
> int sec_btf_id;
> const char *name;
> + const struct btf_type *btf_type;
> bool is_set;
> bool is_weak;
> union {
> @@ -3022,7 +3024,7 @@ static bool sym_is_subprog(const GElf_Sym *sym, int text_shndx)
> static int find_extern_btf_id(const struct btf *btf, const char *ext_name)
> {
> const struct btf_type *t;
> - const char *var_name;
> + const char *tname;
> int i, n;
>
> if (!btf)
> @@ -3032,14 +3034,18 @@ static int find_extern_btf_id(const struct btf *btf, const char *ext_name)
> for (i = 1; i <= n; i++) {
> t = btf__type_by_id(btf, i);
>
> - if (!btf_is_var(t))
> + if (!btf_is_var(t) && !btf_is_func(t))
> continue;
>
> - var_name = btf__name_by_offset(btf, t->name_off);
> - if (strcmp(var_name, ext_name))
> + tname = btf__name_by_offset(btf, t->name_off);
> + if (strcmp(tname, ext_name))
> continue;
>
> - if (btf_var(t)->linkage != BTF_VAR_GLOBAL_EXTERN)
> + if (btf_is_var(t) &&
> + btf_var(t)->linkage != BTF_VAR_GLOBAL_EXTERN)
> + return -EINVAL;
> +
> + if (btf_is_func(t) && btf_func_linkage(t) != BTF_FUNC_EXTERN)
> return -EINVAL;
>
> return i;
> @@ -3199,10 +3205,10 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
> return ext->btf_id;
> }
> t = btf__type_by_id(obj->btf, ext->btf_id);
> + ext->btf_type = t;
ext->btf_type is derived from ext->btf_id and obj->btf (always), so
there is no need for it
> ext->name = btf__name_by_offset(obj->btf, t->name_off);
> ext->sym_idx = i;
> ext->is_weak = GELF_ST_BIND(sym.st_info) == STB_WEAK;
> -
> ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
> if (ext->sec_btf_id <= 0) {
> pr_warn("failed to find BTF for extern '%s' [%d] section: %d\n",
> @@ -3212,6 +3218,34 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
> sec = (void *)btf__type_by_id(obj->btf, ext->sec_btf_id);
> sec_name = btf__name_by_offset(obj->btf, sec->name_off);
>
> + if (btf_is_func(t)) {
there is a KSYMS_SEC handling logic below, let's keep both func and
variables handling together there?
> + const struct btf_type *func_proto;
> +
> + func_proto = btf__type_by_id(obj->btf, t->type);
> + if (!func_proto || !btf_is_func_proto(func_proto)) {
this is implied by BTF format itself, seems a bit redundant
> + pr_warn("extern function %s does not have a valid func_proto\n",
> + ext->name);
> + return -EINVAL;
> + }
> +
> + if (ext->is_weak) {
> + pr_warn("extern weak function %s is unsupported\n",
> + ext->name);
> + return -ENOTSUP;
> + }
> +
> + if (strcmp(sec_name, KSYMS_SEC)) {
> + pr_warn("extern function %s is only supported under %s section\n",
> + ext->name, KSYMS_SEC);
> + return -ENOTSUP;
> + }
> +
> + ksym_sec = sec;
> + ext->type = EXT_KSYM;
> + ext->ksym.type_id = ext->btf_id;
there is skip_mods_and_typedefs in KSYMS_SEC section below, but it
won't have any effect on FUNC_PROTO, so existing logic can be used
as-is
> + continue;
> + }
> +
> if (strcmp(sec_name, KCONFIG_SEC) == 0) {
> kcfg_sec = sec;
> ext->type = EXT_KCFG;
[...]
> +static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
> + struct extern_desc *ext)
> +{
> + int local_func_proto_id, kern_func_proto_id, kern_func_id;
> + const struct btf_type *kern_func;
> + struct btf *kern_btf = NULL;
> + int ret, kern_btf_fd = 0;
> +
> + local_func_proto_id = ext->btf_type->type;
yeah, so this ext->btf_type can be retrieved with
btf__type_by_id(obj->btf, ext->btf_id) here, no need to pollute
extern_desc with extra field
> +
> + kern_func_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC,
> + &kern_btf, &kern_btf_fd);
> + if (kern_func_id < 0) {
> + pr_warn("extern (func ksym) '%s': not found in kernel BTF\n",
> + ext->name);
> + return kern_func_id;
> + }
> +
> + if (kern_btf != obj->btf_vmlinux) {
> + pr_warn("extern (func ksym) '%s': function in kernel module is not supported\n",
> + ext->name);
> + return -ENOTSUP;
> + }
> +
[...]
