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:
- if variable type is `extern void` -- do what we do today (no BTF required)
- 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.