| Message ID | 20221126094530.226629-2-yangjihong1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bpf: Support kernel function call in 32-bit ARM | expand |
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() ? 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.
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. 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. > . >
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 ? > 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. > > . > >
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. >>> . >>> > . >
On Fri, Dec 2, 2022 at 6:58 PM Yang Jihong <yangjihong1@huawei.com> wrote: > > > > 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 kernel returns -14 == EFAULT. That's a completely different issue.
On 2022/12/4 0:40, Alexei Starovoitov wrote: > On Fri, Dec 2, 2022 at 6:58 PM Yang Jihong <yangjihong1@huawei.com> wrote: >> >> >> >> 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 kernel returns -14 == EFAULT. > That's a completely different issue. It's the same problem. The opt_subreg_zext_lo32_rnd_hi32 function fails to check here and returns -EFAULT 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; } ... } > . >
Hello, On 2022/12/5 9:19, Yang Jihong wrote: > > > On 2022/12/4 0:40, Alexei Starovoitov wrote: >> On Fri, Dec 2, 2022 at 6:58 PM Yang Jihong <yangjihong1@huawei.com> >> wrote: >>> >>> >>> >>> 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 kernel returns -14 == EFAULT. >> That's a completely different issue. > It's the same problem. The opt_subreg_zext_lo32_rnd_hi32 function fails > to check here and returns -EFAULT > > 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; > } > ... > } >> . I see that there are emails from the community talking about the same problem, and come up with a solution: https://lore.kernel.org/bpf/20221202103620.1915679-1-bjorn@kernel.org/T/ will remove this patch based on that patch. Thanks, Yang >> > > .
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; case BPF_JMP32: case BPF_ST: return -1; @@ -2430,7 +2468,7 @@ static int insn_def_regno(const struct bpf_insn *insn) /* Return TRUE if INSN has defined any 32-bit value explicitly. */ static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn) { - int dst_reg = insn_def_regno(insn); + int dst_reg = insn_def_regno(env, insn); if (dst_reg == -1) return false; @@ -13335,7 +13373,7 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, int load_reg; insn = insns[adj_idx]; - load_reg = insn_def_regno(&insn); + load_reg = insn_def_regno(env, &insn); if (!aux[adj_idx].zext_dst) { u8 code, class; u32 imm_rnd;
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(-)