From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4941EC43217 for ; Mon, 28 Nov 2022 12:41:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:CC:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=SUP9mPCRNKOej+ncsGYMS2lye1deFTkNONCrqUnufbM=; b=uCX7kVHiptMylY+I05SnjKlIrZ ANAGIcM9eoxKBmyJAMP2YE4pHEspzmxqwb45kWYwSA8rdDnnhX1wM1IuoESDyeASdl0gt6/F4qm5S Y2ezK05DUC8Xagb071Ya5GncLkXYdxjq0yKv7XpDvcMd9zSme9D3sI2OhibaCyg5p/lEdHXbWFhxF ZMhiJiIIMUVF0jEJn24VegtW6ZIBOURhtbZiSTC0oXSiT6gcrhDf2w0h1GyOe73ctEZQBXwcjvE7u C+X+2sSafPtjpp9Hb5gmzA2JmK6RJMiBfm/o//R8RknMB8f79LP2mE8zAfJJwa8j3MHjqjjd+W4f0 aRVKZoxw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ozdQq-001Zzg-7L; Mon, 28 Nov 2022 12:40:20 +0000 Received: from szxga01-in.huawei.com ([45.249.212.187]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ozdQm-001ZwZ-Kr for linux-arm-kernel@lists.infradead.org; Mon, 28 Nov 2022 12:40:19 +0000 Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4NLQ5Z45tRzmW5l; Mon, 28 Nov 2022 20:39:30 +0800 (CST) Received: from kwepemm600003.china.huawei.com (7.193.23.202) by dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Mon, 28 Nov 2022 20:40:08 +0800 Received: from [10.67.111.205] (10.67.111.205) by kwepemm600003.china.huawei.com (7.193.23.202) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Mon, 28 Nov 2022 20:40:07 +0800 Subject: Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension To: Alexei Starovoitov CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , References: <20221126094530.226629-1-yangjihong1@huawei.com> <20221126094530.226629-2-yangjihong1@huawei.com> <20221128015758.aekybr3qlahfopwq@MacBook-Pro-5.local> From: Yang Jihong Message-ID: Date: Mon, 28 Nov 2022 20:40:06 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: <20221128015758.aekybr3qlahfopwq@MacBook-Pro-5.local> Content-Language: en-US X-Originating-IP: [10.67.111.205] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemm600003.china.huawei.com (7.193.23.202) X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221128_044017_078884_2841BFE6 X-CRM114-Status: GOOD ( 31.73 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 >> --- >> 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. > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel