Hello, On 2022/12/5 9:19, Yang Jihong wrote:
I see that there are emails from the community talking about the same problem, and come up with a solution:On 2022/12/4 0:40, Alexei Starovoitov wrote:On Fri, Dec 2, 2022 at 6:58 PM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:It's the same problem. The opt_subreg_zext_lo32_rnd_hi32 function fails to check here and returns -EFAULTOn 2022/11/29 0:41, Alexei Starovoitov wrote:On Mon, Nov 28, 2022 at 4:40 AM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:On 2022/11/28 9:57, Alexei Starovoitov wrote:On Sat, Nov 26, 2022 at 05:45:27PM +0800, Yang Jihong wrote:For ARM32 architecture, if data width of kfunc return value is 32 bits, need to do explicit zero extension for high 32-bit, insn_def_regno shouldreturn dst_reg for BPF_JMP type of BPF_PSEUDO_KFUNC_CALL. Otherwise,opt_subreg_zext_lo32_rnd_hi32 returns -EFAULT, resulting in BPF failure.Signed-off-by: Yang Jihong <yangjihong1@xxxxxxxxxx> ---kernel/bpf/verifier.c | 44 ++++++++++++++++++++++++++++++++++++++++---1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 264b3dc714cc..193ea927aa69 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c@@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog *prog, u32 func_id, u16 offset) sizeof(tab->descs[0]), kfunc_desc_cmp_by_id_off);} +static int kfunc_desc_cmp_by_imm(const void *a, const void *b); + +static const struct bpf_kfunc_desc * +find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm) +{ + struct bpf_kfunc_desc desc = { + .imm = imm, + }; + struct bpf_kfunc_desc_tab *tab; + + tab = prog->aux->kfunc_tab; + return bsearch(&desc, tab->descs, tab->nr_descs, + sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm); +} +static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,s16 offset) {@@ -2342,6 +2357,13 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,*/ if (insn->src_reg == BPF_PSEUDO_CALL) return false; ++ /* Kfunc call will reach here because of insn_has_def32,+ * conservatively return TRUE. + */ + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) + return true; +/* Helper call will reach here because of arg type* check, conservatively return TRUE. */@@ -2405,10 +2427,26 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,} /* Return the regno defined by the insn, or -1. */ -static int insn_def_regno(const struct bpf_insn *insn)+static int insn_def_regno(struct bpf_verifier_env *env, const struct bpf_insn *insn){ switch (BPF_CLASS(insn->code)) { case BPF_JMP: + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { + const struct bpf_kfunc_desc *desc; + + /* The value of desc cannot be NULL */+ desc = find_kfunc_desc_by_imm(env->prog, insn->imm);+ + /* A kfunc can return void.+ * The btf type of the kfunc's return value needs+ * to be checked against "void" first + */ + if (desc->func_model.ret_size == 0) + return -1; + else + return insn->dst_reg; + } + fallthrough;I cannot make any sense of this patch. insn->dst_reg above is 0. The kfunc call doesn't define a register from insn_def_regno() pov. Are you hacking insn_def_regno() to return 0 so that if (WARN_ON(load_reg == -1)) {verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");return -EFAULT; } in opt_subreg_zext_lo32_rnd_hi32() doesn't trigger ? But this verifier message should have been a hint that you need to analyze why zext_dst is set on this kfunc call. Maybe it shouldn't ? Did you analyze the logic of mark_btf_func_reg_size() ?make r0 zext is not caused by mark_btf_func_reg_size. This problem occurs when running the kfunc_call_test_ref_btf_id test case in the 32-bit ARM environment.Why is it not failing on x86-32 ?Use the latest mainline kernel code to test on the x86_32 machine. The test also fails: # ./test_progs -t kfunc_call/kfunc_call_test_ref_btf_id Failed to load bpf_testmod.ko into the kernel: -8 WARNING! Selftests relying on bpf_testmod.ko will be skipped. libbpf: prog 'kfunc_call_test_ref_btf_id': BPF program load failed: Bad address libbpf: prog 'kfunc_call_test_ref_btf_id': -- BEGIN PROG LOAD LOG --processed 25 insns (limit 1000000) max_states_per_insn 0 total_states2 peak_states 2 mark_read 1 -- END PROG LOAD LOG -- libbpf: prog 'kfunc_call_test_ref_btf_id': failed to load: -14 libbpf: failed to load object 'kfunc_call_test' libbpf: failed to load BPF skeleton 'kfunc_call_test': -14 verify_success:FAIL:skel unexpected error: -14 Therefore, this problem also exists on x86_32: "verifier bug. zext_dst is set, but no reg is defined"The kernel returns -14 == EFAULT. That's a completely different issue.opt_subreg_zext_lo32_rnd_hi32 { ... if (WARN_ON(load_reg == -1)) {verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");return -EFAULT; } ... }.
https://lore.kernel.org/bpf/20221202103620.1915679-1-bjorn@xxxxxxxxxx/T/ will remove this patch based on that patch. Thanks, Yang
.
