Message-ID: <20210318233907.r5pxtpe477jumjq6@kafai-mbp.dhcp.thefacebook.com>
Date:   Thu, 18 Mar 2021 16:39:07 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 02/15] bpf: btf: Support parsing extern func

On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote:
> On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@...com> wrote:
> >
> > This patch makes BTF verifier to accept extern func. It is used for
> > allowing bpf program to call a limited set of kernel functions
> > in a later patch.
> >
> > When writing bpf prog, the extern kernel function needs
> > to be declared under a ELF section (".ksyms") which is
> > the same as the current extern kernel variables and that should
> > keep its usage consistent without requiring to remember another
> > section name.
> >
> > For example, in a bpf_prog.c:
> >
> > extern int foo(struct sock *) __attribute__((section(".ksyms")))
> >
> > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1
> >         '(anon)' type_id=18
> > [25] FUNC 'foo' type_id=24 linkage=extern
> > [ ... ]
> > [33] DATASEC '.ksyms' size=0 vlen=1
> >         type_id=25 offset=0 size=0
> >
> > LLVM will put the "func" type into the BTF datasec ".ksyms".
> > The current "btf_datasec_check_meta()" assumes everything under
> > it is a "var" and ensures it has non-zero size ("!vsi->size" test).
> > The non-zero size check is not true for "func".  This patch postpones the
> > "!vsi-size" test from "btf_datasec_check_meta()" to
> > "btf_datasec_resolve()" which has all types collected to decide
> > if a vsi is a "var" or a "func" and then enforce the "vsi->size"
> > differently.
> >
> > If the datasec only has "func", its "t->size" could be zero.
> > Thus, the current "!t->size" test is no longer valid.  The
> > invalid "t->size" will still be caught by the later
> > "last_vsi_end_off > t->size" check.   This patch also takes this
> > chance to consolidate other "t->size" tests ("vsi->offset >= t->size"
> > "vsi->size > t->size", and "t->size < sum") into the existing
> > "last_vsi_end_off > t->size" test.
> >
> > The LLVM will also put those extern kernel function as an extern
> > linkage func in the BTF:
> >
> > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1
> >         '(anon)' type_id=18
> > [25] FUNC 'foo' type_id=24 linkage=extern
> >
> > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta().
> > Also extern kernel function declaration does not
> > necessary have arg name. Another change in btf_func_check() is
> > to allow extern function having no arg name.
> >
> > The btf selftest is adjusted accordingly.  New tests are also added.
> >
> > The required LLVM patch: https://reviews.llvm.org/D93563 
> >
> > Signed-off-by: Martin KaFai Lau <kafai@...com>
> > ---
> 
> High-level question about EXTERN functions in DATASEC. Does kernel
> need to see them under DATASEC? What if libbpf just removed all EXTERN
> funcs from under DATASEC and leave them as "free-floating" EXTERN
> FUNCs in BTF.
> 
> We need to tag EXTERNs with DATASECs mainly for libbpf to know whether
> it's .kconfig or .ksym or other type of externs. Does kernel need to
> care?
Although the kernel does not need to know, since the a legit llvm generates it,
I go with a proper support in the kernel (e.g. bpftool btf dump can better
reflect what was there).

> 
> >  kernel/bpf/btf.c                             |  52 ++++---
> >  tools/testing/selftests/bpf/prog_tests/btf.c | 154 ++++++++++++++++++-
> >  2 files changed, 178 insertions(+), 28 deletions(-)
> >
> 
> [...]
> 
> > @@ -3611,9 +3594,28 @@ static int btf_datasec_resolve(struct btf_verifier_env *env,
> >                 u32 var_type_id = vsi->type, type_id, type_size = 0;
> >                 const struct btf_type *var_type = btf_type_by_id(env->btf,
> >                                                                  var_type_id);
> > -               if (!var_type || !btf_type_is_var(var_type)) {
> > +               if (!var_type) {
> > +                       btf_verifier_log_vsi(env, v->t, vsi,
> > +                                            "type not found");
> > +                       return -EINVAL;
> > +               }
> > +
> > +               if (btf_type_is_func(var_type)) {
> > +                       if (vsi->size || vsi->offset) {
> > +                               btf_verifier_log_vsi(env, v->t, vsi,
> > +                                                    "Invalid size/offset");
> > +                               return -EINVAL;
> > +                       }
> > +                       continue;
> > +               } else if (btf_type_is_var(var_type)) {
> > +                       if (!vsi->size) {
> > +                               btf_verifier_log_vsi(env, v->t, vsi,
> > +                                                    "Invalid size");
> > +                               return -EINVAL;
> > +                       }
> > +               } else {
> >                         btf_verifier_log_vsi(env, v->t, vsi,
> > -                                            "Not a VAR kind member");
> > +                                            "Neither a VAR nor a FUNC");
> >                         return -EINVAL;
> 
> can you please structure it as follow (I think it is bit easier to
> follow the logic then):
> 
> if (btf_type_is_func()) {
>    ...
>    continue; /* no extra checks */
> }
> 
> if (!btf_type_is_var()) {
>    /* bad, complain, exit */
>    return -EINVAL;
> }
> 
> /* now we deal with extra checks for variables */
> 
> That way variable checks are kept all in one place.
> 
> Also a question: is that ok to enable non-extern functions under
> DATASEC? Maybe, but that wasn't explicitly mentioned.
The patch does not check.  We could reject that for now.

> 
> >                 }
> >
> > @@ -3849,9 +3851,11 @@ static int btf_func_check(struct btf_verifier_env *env,
> >         const struct btf_param *args;
> >         const struct btf *btf;
> >         u16 nr_args, i;
> > +       bool is_extern;
> >
> >         btf = env->btf;
> >         proto_type = btf_type_by_id(btf, t->type);
> > +       is_extern = btf_type_vlen(t) == BTF_FUNC_EXTERN;
> 
> using btf_type_vlen(t) for getting func linkage is becoming more and
> more confusing. Would it be terrible to have btf_func_linkage(t)
> helper instead?
I have it in my local v2.  and also just return when it is extern.

> 
> >
> >         if (!proto_type || !btf_type_is_func_proto(proto_type)) {
> >                 btf_verifier_log_type(env, t, "Invalid type_id");
> > @@ -3861,7 +3865,7 @@ static int btf_func_check(struct btf_verifier_env *env,
> >         args = (const struct btf_param *)(proto_type + 1);
> >         nr_args = btf_type_vlen(proto_type);
> >         for (i = 0; i < nr_args; i++) {
> > -               if (!args[i].name_off && args[i].type) {
> > +               if (!is_extern && !args[i].name_off && args[i].type) {
> >                         btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1);
> >                         return -EINVAL;
> >                 }
> 
> [...]

Powered by blists - more mailing lists