Messages in this thread | | | Subject | Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension | From | Yang Jihong <> | Date | Sat, 3 Dec 2022 10:58:44 +0800 |
| |
On 2022/11/29 0:41, Alexei Starovoitov wrote: > On Mon, Nov 28, 2022 at 4:40 AM Yang Jihong <yangjihong1@huawei.com> 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 should >>>> return 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@huawei.com> >>>> --- >>>> 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_states 2 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 bpf prog is as follows: >> int kfunc_call_test_ref_btf_id(struct __sk_buff *skb) >> { >> struct prog_test_ref_kfunc *pt; >> unsigned long s = 0; >> int ret = 0; >> >> pt = bpf_kfunc_call_test_acquire(&s); >> if (pt) { >> // here, do_check clears the upper 32bits of r0 through: >> // check_alu_op >> // ->check_reg_arg >> // ->mark_insn_zext >> if (pt->a != 42 || pt->b != 108) >> ret = -1; >> bpf_kfunc_call_test_release(pt); >> } >> return ret; >> } >> >>> >>> Before producing any patches please understand the logic fully. >>> Your commit log >>> "insn_def_regno should >>> return dst_reg for BPF_JMP type of BPF_PSEUDO_KFUNC_CALL." >>> >>> Makes no sense to me, since dst_reg is unused in JMP insn. >>> There is no concept of a src or dst register in a JMP insn. >>> >>> 32-bit x86 supports calling kfuncs. See emit_kfunc_call(). >>> And we don't have this "verifier bug. zext_dst is set" issue there, right? >>> But what you're saying in the commit log: >>> "if data width of kfunc return value is 32 bits" >>> should have been applicable to x86-32 as well. >>> So please start with a test that demonstrates the issue on x86-32 and >>> then we can discuss the way to fix it. >>> >>> The patch 2 sort-of makes sense. >>> >>> For patch 3 pls add new test funcs to bpf_testmod. >>> We will move all of them from net/bpf/test_run.c to bpf_testmod eventually. >>> . >>> > . >
| |