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