Re: [PATCH bpf-next 12/15] libbpf: Support extern kernel function

[Martin KaFai Lau <kafai@fb.com>]

On Thu, Mar 18, 2021 at 09:11:39PM -0700, Andrii Nakryiko wrote:
> On Tue, Mar 16, 2021 at 12:02 AM Martin KaFai Lau <kafai@fb.com> 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@fb.com>
> > ---
> >  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.
The ret < 0 case? In that case, size is 0.

or there are cases that a section has no vars but the size should not be 0?

> 
> >
> >         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.
will do.

> 
> > +
> >  #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
It is for easier btf_is_var() check later instead of going through
another btf__type_by_id().

yeah, I will make a few btf__type_by_id() calls in v2.

> 
> >                 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?
It is to keep the indentation manageable
and also most of the things doing here is not
sharable with variables.

Sure. I can move it 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
It has already been checked?

> 
> > +                               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
func id is used here to keep what ksyms.type_id means:
/* local btf_id of the ksym extern's type. */

The kernel extern type here should be func instead of func_proto.
func_proto cannot be extern.

> 
> > +                       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;
> > +       }
> > +
> 
> [...]