From: Yang Jihong <yangjihong1@huawei.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: <ast@kernel.org>, <daniel@iogearbox.net>, <andrii@kernel.org>,
<martin.lau@linux.dev>, <song@kernel.org>, <yhs@fb.com>,
<john.fastabend@gmail.com>, <kpsingh@kernel.org>,
<sdf@google.com>, <haoluo@google.com>, <jolsa@kernel.org>,
<illusionist.neo@gmail.com>, <linux@armlinux.org.uk>,
<davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
<pabeni@redhat.com>, <mykolal@fb.com>, <shuah@kernel.org>,
<benjamin.tissoires@redhat.com>, <memxor@gmail.com>,
<colin.i.king@gmail.com>, <asavkov@redhat.com>, <delyank@fb.com>,
<bpf@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
Date: Mon, 28 Nov 2022 20:40:06 +0800 [thread overview]
Message-ID: <dc9d1823-80f2-e2d9-39a8-c39b6f52dec5@huawei.com> (raw)
In-Reply-To: <20221128015758.aekybr3qlahfopwq@MacBook-Pro-5.local>
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.
> .
>
next prev parent reply other threads:[~2022-11-28 12:40 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-26 9:45 [PATCH bpf-next v3 0/4] bpf: Support kernel function call in 32-bit ARM Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension Yang Jihong
2022-11-28 1:57 ` Alexei Starovoitov
2022-11-28 12:40 ` Yang Jihong [this message]
2022-11-28 16:41 ` Alexei Starovoitov
2022-12-03 2:58 ` Yang Jihong
2022-12-03 16:40 ` Alexei Starovoitov
2022-12-05 1:19 ` Yang Jihong
2022-12-07 8:49 ` Yang Jihong
2022-12-02 4:11 ` kernel test robot
2022-11-26 9:45 ` [PATCH bpf-next v3 2/4] bpf: Add kernel function call support in 32-bit ARM for EABI Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 3/4] bpf:selftests: Add kfunc_call test for mixing 32-bit and 64-bit parameters Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 4/4] bpf: Fix comment error in fixup_kfunc_call function Yang Jihong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dc9d1823-80f2-e2d9-39a8-c39b6f52dec5@huawei.com \
--to=yangjihong1@huawei.com \
--cc=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=asavkov@redhat.com \
--cc=ast@kernel.org \
--cc=benjamin.tissoires@redhat.com \
--cc=bpf@vger.kernel.org \
--cc=colin.i.king@gmail.com \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=delyank@fb.com \
--cc=edumazet@google.com \
--cc=haoluo@google.com \
--cc=illusionist.neo@gmail.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=martin.lau@linux.dev \
--cc=memxor@gmail.com \
--cc=mykolal@fb.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sdf@google.com \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).