Messages in this thread | | | From | Hao Luo <> | Date | Mon, 20 Jul 2020 10:03:05 -0700 | Subject | Re: [RFC PATCH bpf-next 1/2] bpf: BTF support for __ksym externs |
| |
Andrii,
Thanks for taking a look at this. You comments are clear, I will fix them in v2.
> Also, in the next version, please split kernel part and libbpf part > into separate patches. >
Got it. Will do.
> I don't think that's the right approach. It can't be the best effort. > It's actually pretty clear when a user wants a BTF-based variable with > ability to do direct memory access vs __ksym address that we have > right now: variable type info. In your patch you are only looking up > variable by name, but it needs to be more elaborate logic: > > 1. if variable type is `extern void` -- do what we do today (no BTF required) > 2. if the variable type is anything but `extern void`, then find that > variable in BTF. If no BTF or variable is not found -- hard error with > detailed enough message about what we expected to find in kernel BTF. > 3. If such a variable is found in the kernel, then might be a good > idea to additionally check type compatibility (e.g., struct/union > should match struct/union, int should match int, typedefs should get > resolved to underlying type, etc). I don't think deep comparison of > structs is right, though, due to CO-RE, so just high-level > compatibility checks to prevent the most obvious mistakes. >
Ack.
> > > > Also note since we need to carry the ksym's address (64bits) as well as > > its btf_id (32bits), pseudo_btf_id uses ld_imm64's both imm and off > > fields. > > For BTF-enabled ksyms, libbpf doesn't need to provide symbol address, > kernel will find it and substitute it, so BTF ID is the only > parameter. Thus it can just go into the imm field (and simplify > ldimm64 validation logic a bit). >
Ack.
> > /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative > > * offset to another bpf function > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 3c1efc9d08fd..3c925957b9b6 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -7131,15 +7131,29 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn) > > verbose(env, "invalid BPF_LD_IMM insn\n"); > > return -EINVAL; > > } > > + err = check_reg_arg(env, insn->dst_reg, DST_OP); > > + if (err) > > + return err; > > + > > + /* > > + * BPF_PSEUDO_BTF_ID insn's off fields carry the ksym's btf_id, so its > > + * handling has to come before the reserved field check. > > + */ > > + if (insn->src_reg == BPF_PSEUDO_BTF_ID) { > > + u32 id = ((u32)(insn + 1)->off << 16) | (u32)insn->off; > > + const struct btf_type *t = btf_type_by_id(btf_vmlinux, id); > > + > > This is the kernel, we should be paranoid and assume the hackers want > to do bad things. So check t for NULL. Check that it's actually a > BTF_KIND_VAR. Check the name, find ksym addr, etc. >
Ack.
| |