Message ID | 8a0b9f7e4fe208a8b518c0c4310472f99d9fdb55.1668876211.git.christophe.leroy@csgroup.eu |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp830113wrr; Sat, 19 Nov 2022 10:56:58 -0800 (PST) X-Google-Smtp-Source: AA0mqf5zIhDtfjufQk3HWT9G3NzCLamCmrVp6zWor+Rg+OQ8hDeD0Smmt9PnRAORAivuRKWKVZX2 X-Received: by 2002:a17:907:b60f:b0:787:8884:78df with SMTP id vl15-20020a170907b60f00b00787888478dfmr9942993ejc.246.1668884218738; Sat, 19 Nov 2022 10:56:58 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668884218; cv=none; d=google.com; s=arc-20160816; b=Y5cR53FHW1mtMQuajXtVp7BVeR1Fshwjz0k8JXWTxjaL4jHqfredNypAtXNDBNUqBQ KgAEuwJNpSA9Yn9Ija2ObCinZs2o/mLMWVKEglNuI5lihOMdYAN3/Li0d0QtpJt7UfBo P0ykJaHFL95QT2aVwhzN+Em1aj7Fk5/vyf54zFfOBC1VkYXfCceJEuETp/op9qBjCyay SowFO6aBqVk5olERQSTdgOV2hLyQ066WmuPox81uOEMhBW0MRy0oJw4WzOB8mwKsXoQy 3dLvFwTG1OqgCx92GKWt5WpQaA2BxgS9Pw2xD1lsaN45cweEr7FckfB2hJI0/ntyXcpB 7POQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=k3cGj7JLpVrM5eUl36UJItzjCkcgygoKepQKfItjxmI=; b=LW/1d/0M9l1TdvMkFlCpwOZIvEHt2X7P1xx//35UdY+TrwdJsjnBOTsfD9HT9eT751 XkPbSg3la4OfvXC1NV00fuKckGI8/qlNOwL/CfYJc+dKxNfvguPNRWp0kJvAWZzxkXIi rRr64c9vuuykbfSsKSiXx5Rfsm7duYP5OUBwx9UyNS9e5Z/5Q3dyUyTVdCYDC6XGS7iK AAIgUjDiplDWqYXpOhJP1phsnsUMgkHvhT11MdnJUbqX9Y2jrVX/wqI+QFyRSPb1ICrr e6V1rI2xbSpKnJgoVfAV/sG7FIxsiN5Vz//WHh/ttW1g4WpWETkRH+YQ1XcO0kILR1Gb WrQA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id pw27-20020a17090720bb00b007ae0211844dsi5520238ejb.937.2022.11.19.10.56.34; Sat, 19 Nov 2022 10:56:58 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234216AbiKSSVY (ORCPT <rfc822;kkmonlee@gmail.com> + 99 others); Sat, 19 Nov 2022 13:21:24 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43506 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233670AbiKSSVW (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 19 Nov 2022 13:21:22 -0500 Received: from pegase2.c-s.fr (pegase2.c-s.fr [93.17.235.10]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4680D13D43; Sat, 19 Nov 2022 10:21:19 -0800 (PST) Received: from localhost (mailhub3.si.c-s.fr [172.26.127.67]) by localhost (Postfix) with ESMTP id 4NF2650XQDz9sZd; Sat, 19 Nov 2022 19:21:17 +0100 (CET) X-Virus-Scanned: amavisd-new at c-s.fr Received: from pegase2.c-s.fr ([172.26.127.65]) by localhost (pegase2.c-s.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9dP50iU9p0hz; Sat, 19 Nov 2022 19:21:16 +0100 (CET) Received: from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192]) by pegase2.c-s.fr (Postfix) with ESMTP id 4NF26460kRz9sZY; Sat, 19 Nov 2022 19:21:16 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by messagerie.si.c-s.fr (Postfix) with ESMTP id AE2AD8B76C; Sat, 19 Nov 2022 19:21:16 +0100 (CET) X-Virus-Scanned: amavisd-new at c-s.fr Received: from messagerie.si.c-s.fr ([127.0.0.1]) by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023) with ESMTP id ZSS9eVuynq4e; Sat, 19 Nov 2022 19:21:16 +0100 (CET) Received: from PO20335.IDSI0.si.c-s.fr (unknown [192.168.4.152]) by messagerie.si.c-s.fr (Postfix) with ESMTP id 5A5D48B763; Sat, 19 Nov 2022 19:21:16 +0100 (CET) Received: from PO20335.IDSI0.si.c-s.fr (localhost [127.0.0.1]) by PO20335.IDSI0.si.c-s.fr (8.17.1/8.16.1) with ESMTPS id 2AJHJgAl2482924 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Sat, 19 Nov 2022 18:19:42 +0100 Received: (from chleroy@localhost) by PO20335.IDSI0.si.c-s.fr (8.17.1/8.17.1/Submit) id 2AJHJe3a2482922; Sat, 19 Nov 2022 18:19:40 +0100 X-Authentication-Warning: PO20335.IDSI0.si.c-s.fr: chleroy set sender to christophe.leroy@csgroup.eu using -f From: Christophe Leroy <christophe.leroy@csgroup.eu> To: Michael Ellerman <mpe@ellerman.id.au>, Nicholas Piggin <npiggin@gmail.com>, "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>, John Fastabend <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, stable@vger.kernel.org Subject: [PATCH] powerpc/bpf/32: Fix Oops on tail call tests Date: Sat, 19 Nov 2022 18:19:35 +0100 Message-Id: <8a0b9f7e4fe208a8b518c0c4310472f99d9fdb55.1668876211.git.christophe.leroy@csgroup.eu> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 X-Developer-Signature: v=1; a=ed25519-sha256; t=1668878372; l=6038; s=20211009; h=from:subject:message-id; bh=yCeqy7mTPVbgcrSwBiCurTBvvyery5JpUQSfv916m3k=; b=5WQZbI9f+2gy/zQTO6eMU+RJE/pGup2cXIDkBb5INfBfxelfMRDe5qL5ixybklkhLpOa4D4VBlmw cbraNIixAUs5Nx580UioNSwkJxCM+JbWlo3vN5aHiJ5hWSbqRr1E X-Developer-Key: i=christophe.leroy@csgroup.eu; a=ed25519; pk=HIzTzUj91asvincQGOFx6+ZF5AoUuP9GdOtQChs7Mm0= Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749951938333097488?= X-GMAIL-MSGID: =?utf-8?q?1749951938333097488?= |
Series |
powerpc/bpf/32: Fix Oops on tail call tests
|
|
Commit Message
Christophe Leroy
Nov. 19, 2022, 5:19 p.m. UTC
test_bpf tail call tests end up as:
test_bpf: #0 Tail call leaf jited:1 85 PASS
test_bpf: #1 Tail call 2 jited:1 111 PASS
test_bpf: #2 Tail call 3 jited:1 145 PASS
test_bpf: #3 Tail call 4 jited:1 170 PASS
test_bpf: #4 Tail call load/store leaf jited:1 190 PASS
test_bpf: #5 Tail call load/store jited:1
BUG: Unable to handle kernel data access on write at 0xf1b4e000
Faulting instruction address: 0xbe86b710
Oops: Kernel access of bad area, sig: 11 [#1]
BE PAGE_SIZE=4K MMU=Hash PowerMac
Modules linked in: test_bpf(+)
CPU: 0 PID: 97 Comm: insmod Not tainted 6.1.0-rc4+ #195
Hardware name: PowerMac3,1 750CL 0x87210 PowerMac
NIP: be86b710 LR: be857e88 CTR: be86b704
REGS: f1b4df20 TRAP: 0300 Not tainted (6.1.0-rc4+)
MSR: 00009032 <EE,ME,IR,DR,RI> CR: 28008242 XER: 00000000
DAR: f1b4e000 DSISR: 42000000
GPR00: 00000001 f1b4dfe0 c11d2280 00000000 00000000 00000000 00000002 00000000
GPR08: f1b4e000 be86b704 f1b4e000 00000000 00000000 100d816a f2440000 fe73baa8
GPR16: f2458000 00000000 c1941ae4 f1fe2248 00000045 c0de0000 f2458030 00000000
GPR24: 000003e8 0000000f f2458000 f1b4dc90 3e584b46 00000000 f24466a0 c1941a00
NIP [be86b710] 0xbe86b710
LR [be857e88] __run_one+0xec/0x264 [test_bpf]
Call Trace:
[f1b4dfe0] [00000002] 0x2 (unreliable)
Instruction dump:
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
---[ end trace 0000000000000000 ]---
This is a tentative to write above the stack. The problem is encoutered
with tests added by commit 38608ee7b690 ("bpf, tests: Add load store
test case for tail call")
This happens because tail call is done to a BPF prog with a different
stack_depth. At the time being, the stack is kept as is when the caller
tail calls its callee. But at exit, the callee restores the stack based
on its own properties. Therefore here, at each run, r1 is erroneously
increased by 32 - 16 = 16 bytes.
This was done that way in order to pass the tail call count from caller
to callee through the stack. As powerpc32 doesn't have a red zone in
the stack, it was necessary the maintain the stack as is for the tail
call. But it was not anticipated that the BPF frame size could be
different.
Let's take a new approach. Use register r0 to carry the tail call count
during the tail call, and save it into the stack at function entry if
required. That's a deviation from the ppc32 ABI, but after all the way
tail calls are implemented is already not in accordance with the ABI.
With the fix, tail call tests are now successfull:
test_bpf: #0 Tail call leaf jited:1 53 PASS
test_bpf: #1 Tail call 2 jited:1 115 PASS
test_bpf: #2 Tail call 3 jited:1 154 PASS
test_bpf: #3 Tail call 4 jited:1 165 PASS
test_bpf: #4 Tail call load/store leaf jited:1 101 PASS
test_bpf: #5 Tail call load/store jited:1 141 PASS
test_bpf: #6 Tail call error path, max count reached jited:1 994 PASS
test_bpf: #7 Tail call count preserved across function calls jited:1 140975 PASS
test_bpf: #8 Tail call error path, NULL target jited:1 110 PASS
test_bpf: #9 Tail call error path, index out of range jited:1 69 PASS
test_bpf: test_tail_calls: Summary: 10 PASSED, 0 FAILED, [10/10 JIT'ed]
Fixes: 51c66ad849a7 ("powerpc/bpf: Implement extended BPF on PPC32")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/net/bpf_jit_comp32.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
Comments
Christophe Leroy wrote: > test_bpf tail call tests end up as: > > test_bpf: #0 Tail call leaf jited:1 85 PASS > test_bpf: #1 Tail call 2 jited:1 111 PASS > test_bpf: #2 Tail call 3 jited:1 145 PASS > test_bpf: #3 Tail call 4 jited:1 170 PASS > test_bpf: #4 Tail call load/store leaf jited:1 190 PASS > test_bpf: #5 Tail call load/store jited:1 > BUG: Unable to handle kernel data access on write at 0xf1b4e000 > Faulting instruction address: 0xbe86b710 > Oops: Kernel access of bad area, sig: 11 [#1] > BE PAGE_SIZE=4K MMU=Hash PowerMac > Modules linked in: test_bpf(+) > CPU: 0 PID: 97 Comm: insmod Not tainted 6.1.0-rc4+ #195 > Hardware name: PowerMac3,1 750CL 0x87210 PowerMac > NIP: be86b710 LR: be857e88 CTR: be86b704 > REGS: f1b4df20 TRAP: 0300 Not tainted (6.1.0-rc4+) > MSR: 00009032 <EE,ME,IR,DR,RI> CR: 28008242 XER: 00000000 > DAR: f1b4e000 DSISR: 42000000 > GPR00: 00000001 f1b4dfe0 c11d2280 00000000 00000000 00000000 00000002 00000000 > GPR08: f1b4e000 be86b704 f1b4e000 00000000 00000000 100d816a f2440000 fe73baa8 > GPR16: f2458000 00000000 c1941ae4 f1fe2248 00000045 c0de0000 f2458030 00000000 > GPR24: 000003e8 0000000f f2458000 f1b4dc90 3e584b46 00000000 f24466a0 c1941a00 > NIP [be86b710] 0xbe86b710 > LR [be857e88] __run_one+0xec/0x264 [test_bpf] > Call Trace: > [f1b4dfe0] [00000002] 0x2 (unreliable) > Instruction dump: > XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX > XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX > ---[ end trace 0000000000000000 ]--- > > This is a tentative to write above the stack. The problem is encoutered > with tests added by commit 38608ee7b690 ("bpf, tests: Add load store > test case for tail call") > > This happens because tail call is done to a BPF prog with a different > stack_depth. At the time being, the stack is kept as is when the caller > tail calls its callee. But at exit, the callee restores the stack based > on its own properties. Therefore here, at each run, r1 is erroneously > increased by 32 - 16 = 16 bytes. > > This was done that way in order to pass the tail call count from caller > to callee through the stack. As powerpc32 doesn't have a red zone in > the stack, it was necessary the maintain the stack as is for the tail > call. But it was not anticipated that the BPF frame size could be > different. > > Let's take a new approach. Use register r0 to carry the tail call count > during the tail call, and save it into the stack at function entry if > required. That's a deviation from the ppc32 ABI, but after all the way > tail calls are implemented is already not in accordance with the ABI. Can we pass the tail call count in r4 instead? > > With the fix, tail call tests are now successfull: > > test_bpf: #0 Tail call leaf jited:1 53 PASS > test_bpf: #1 Tail call 2 jited:1 115 PASS > test_bpf: #2 Tail call 3 jited:1 154 PASS > test_bpf: #3 Tail call 4 jited:1 165 PASS > test_bpf: #4 Tail call load/store leaf jited:1 101 PASS > test_bpf: #5 Tail call load/store jited:1 141 PASS > test_bpf: #6 Tail call error path, max count reached jited:1 994 PASS > test_bpf: #7 Tail call count preserved across function calls jited:1 140975 PASS > test_bpf: #8 Tail call error path, NULL target jited:1 110 PASS > test_bpf: #9 Tail call error path, index out of range jited:1 69 PASS > test_bpf: test_tail_calls: Summary: 10 PASSED, 0 FAILED, [10/10 JIT'ed] > > Fixes: 51c66ad849a7 ("powerpc/bpf: Implement extended BPF on PPC32") > Cc: stable@vger.kernel.org > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/net/bpf_jit_comp32.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c > index 43f1c76d48ce..97e75b8181ca 100644 > --- a/arch/powerpc/net/bpf_jit_comp32.c > +++ b/arch/powerpc/net/bpf_jit_comp32.c > @@ -115,21 +115,19 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx) > > /* First arg comes in as a 32 bits pointer. */ > EMIT(PPC_RAW_MR(bpf_to_ppc(BPF_REG_1), _R3)); > - EMIT(PPC_RAW_LI(bpf_to_ppc(BPF_REG_1) - 1, 0)); > + EMIT(PPC_RAW_LI(_R0, 0)); > + > +#define BPF_TAILCALL_PROLOGUE_SIZE 8 > + > EMIT(PPC_RAW_STWU(_R1, _R1, -BPF_PPC_STACKFRAME(ctx))); > > /* > - * Initialize tail_call_cnt in stack frame if we do tail calls. > - * Otherwise, put in NOPs so that it can be skipped when we are > - * invoked through a tail call. > + * Save tail_call_cnt in stack frame if we do tail calls. > */ > if (ctx->seen & SEEN_TAILCALL) > - EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_1) - 1, _R1, > - bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); > - else > - EMIT(PPC_RAW_NOP()); > + EMIT(PPC_RAW_STW(_R0, _R1, bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); > > -#define BPF_TAILCALL_PROLOGUE_SIZE 16 > + EMIT(PPC_RAW_LI(bpf_to_ppc(BPF_REG_1) - 1, 0)); > > /* > * We need a stack frame, but we don't necessarily need to > @@ -244,7 +242,6 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o > EMIT(PPC_RAW_RLWINM(_R3, b2p_index, 2, 0, 29)); > EMIT(PPC_RAW_ADD(_R3, _R3, b2p_bpf_array)); > EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_array, ptrs))); > - EMIT(PPC_RAW_STW(_R0, _R1, bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); > > /* > * if (prog == NULL) > @@ -257,20 +254,20 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o > EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_prog, bpf_func))); > > if (ctx->seen & SEEN_FUNC) > - EMIT(PPC_RAW_LWZ(_R0, _R1, BPF_PPC_STACKFRAME(ctx) + PPC_LR_STKOFF)); > + EMIT(PPC_RAW_LWZ(_R5, _R1, BPF_PPC_STACKFRAME(ctx) + PPC_LR_STKOFF)); > > EMIT(PPC_RAW_ADDIC(_R3, _R3, BPF_TAILCALL_PROLOGUE_SIZE)); > > if (ctx->seen & SEEN_FUNC) > - EMIT(PPC_RAW_MTLR(_R0)); > + EMIT(PPC_RAW_MTLR(_R5)); Should we explicitly zero-out _R5 after this? You can move the above PPC_RAW_LWZ() and PPC_RAW_MTLR() instructions, as well as the ADDI below for r1 into bpf_jit_emit_common_epilogue() and not have to repeat those here. - Naveen > > EMIT(PPC_RAW_MTCTR(_R3)); > > - EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_1))); > - > /* tear restore NVRs, ... */ > bpf_jit_emit_common_epilogue(image, ctx); > > + EMIT(PPC_RAW_ADDI(_R1, _R1, BPF_PPC_STACKFRAME(ctx))); > + > EMIT(PPC_RAW_BCTR()); > > /* out: */ > -- > 2.38.1 > >
Le 22/11/2022 à 08:33, Naveen N. Rao a écrit : > Christophe Leroy wrote: >> test_bpf tail call tests end up as: >> >> test_bpf: #0 Tail call leaf jited:1 85 PASS >> test_bpf: #1 Tail call 2 jited:1 111 PASS >> test_bpf: #2 Tail call 3 jited:1 145 PASS >> test_bpf: #3 Tail call 4 jited:1 170 PASS >> test_bpf: #4 Tail call load/store leaf jited:1 190 PASS >> test_bpf: #5 Tail call load/store jited:1 >> BUG: Unable to handle kernel data access on write at 0xf1b4e000 >> Faulting instruction address: 0xbe86b710 >> Oops: Kernel access of bad area, sig: 11 [#1] >> BE PAGE_SIZE=4K MMU=Hash PowerMac >> Modules linked in: test_bpf(+) >> CPU: 0 PID: 97 Comm: insmod Not tainted 6.1.0-rc4+ #195 >> Hardware name: PowerMac3,1 750CL 0x87210 PowerMac >> NIP: be86b710 LR: be857e88 CTR: be86b704 >> REGS: f1b4df20 TRAP: 0300 Not tainted (6.1.0-rc4+) >> MSR: 00009032 <EE,ME,IR,DR,RI> CR: 28008242 XER: 00000000 >> DAR: f1b4e000 DSISR: 42000000 >> GPR00: 00000001 f1b4dfe0 c11d2280 00000000 00000000 00000000 >> 00000002 00000000 >> GPR08: f1b4e000 be86b704 f1b4e000 00000000 00000000 100d816a >> f2440000 fe73baa8 >> GPR16: f2458000 00000000 c1941ae4 f1fe2248 00000045 c0de0000 >> f2458030 00000000 >> GPR24: 000003e8 0000000f f2458000 f1b4dc90 3e584b46 00000000 >> f24466a0 c1941a00 >> NIP [be86b710] 0xbe86b710 >> LR [be857e88] __run_one+0xec/0x264 [test_bpf] >> Call Trace: >> [f1b4dfe0] [00000002] 0x2 (unreliable) >> Instruction dump: >> XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX >> XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX >> ---[ end trace 0000000000000000 ]--- >> >> This is a tentative to write above the stack. The problem is encoutered >> with tests added by commit 38608ee7b690 ("bpf, tests: Add load store >> test case for tail call") >> >> This happens because tail call is done to a BPF prog with a different >> stack_depth. At the time being, the stack is kept as is when the caller >> tail calls its callee. But at exit, the callee restores the stack based >> on its own properties. Therefore here, at each run, r1 is erroneously >> increased by 32 - 16 = 16 bytes. >> >> This was done that way in order to pass the tail call count from caller >> to callee through the stack. As powerpc32 doesn't have a red zone in >> the stack, it was necessary the maintain the stack as is for the tail >> call. But it was not anticipated that the BPF frame size could be >> different. >> >> Let's take a new approach. Use register r0 to carry the tail call count >> during the tail call, and save it into the stack at function entry if >> required. That's a deviation from the ppc32 ABI, but after all the way >> tail calls are implemented is already not in accordance with the ABI. > > Can we pass the tail call count in r4 instead? It's a bit tricky. When entering the function through the normal entry point, the input parameter is a 32 bits pointer and is in r3. But at the begining of the function it gets moved to r4 and r3 is cleared because it becomes a 64 bits parameter. When using the tailcall entry point, it is already in r4, and until now r3 was containing garbage, with this patch r3 gets cleared as well. We could move the input pointer back into r3 for the tailcall as well, but it would mean unnecessary register move. Or we can use r3 for the tailcall counter. Or I should make r3,r4 a proper 64 bits param (Meaning clearing r3 before the call instead of doing it at function tailcall entry), and use r5 for the tailcall counter. Maybe that's the cleanest. > >> >> With the fix, tail call tests are now successfull: >> >> test_bpf: #0 Tail call leaf jited:1 53 PASS >> test_bpf: #1 Tail call 2 jited:1 115 PASS >> test_bpf: #2 Tail call 3 jited:1 154 PASS >> test_bpf: #3 Tail call 4 jited:1 165 PASS >> test_bpf: #4 Tail call load/store leaf jited:1 101 PASS >> test_bpf: #5 Tail call load/store jited:1 141 PASS >> test_bpf: #6 Tail call error path, max count reached jited:1 994 PASS >> test_bpf: #7 Tail call count preserved across function calls jited:1 >> 140975 PASS >> test_bpf: #8 Tail call error path, NULL target jited:1 110 PASS >> test_bpf: #9 Tail call error path, index out of range jited:1 69 PASS >> test_bpf: test_tail_calls: Summary: 10 PASSED, 0 FAILED, [10/10 JIT'ed] >> >> Fixes: 51c66ad849a7 ("powerpc/bpf: Implement extended BPF on PPC32") >> Cc: stable@vger.kernel.org >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/net/bpf_jit_comp32.c | 25 +++++++++++-------------- >> 1 file changed, 11 insertions(+), 14 deletions(-) >> >> diff --git a/arch/powerpc/net/bpf_jit_comp32.c >> b/arch/powerpc/net/bpf_jit_comp32.c >> index 43f1c76d48ce..97e75b8181ca 100644 >> --- a/arch/powerpc/net/bpf_jit_comp32.c >> +++ b/arch/powerpc/net/bpf_jit_comp32.c >> @@ -115,21 +115,19 @@ void bpf_jit_build_prologue(u32 *image, struct >> codegen_context *ctx) >> >> /* First arg comes in as a 32 bits pointer. */ >> EMIT(PPC_RAW_MR(bpf_to_ppc(BPF_REG_1), _R3)); >> - EMIT(PPC_RAW_LI(bpf_to_ppc(BPF_REG_1) - 1, 0)); >> + EMIT(PPC_RAW_LI(_R0, 0)); >> + >> +#define BPF_TAILCALL_PROLOGUE_SIZE 8 >> + >> EMIT(PPC_RAW_STWU(_R1, _R1, -BPF_PPC_STACKFRAME(ctx))); >> >> /* >> - * Initialize tail_call_cnt in stack frame if we do tail calls. >> - * Otherwise, put in NOPs so that it can be skipped when we are >> - * invoked through a tail call. >> + * Save tail_call_cnt in stack frame if we do tail calls. >> */ >> if (ctx->seen & SEEN_TAILCALL) >> - EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_1) - 1, _R1, >> - bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); >> - else >> - EMIT(PPC_RAW_NOP()); >> + EMIT(PPC_RAW_STW(_R0, _R1, bpf_jit_stack_offsetof(ctx, >> BPF_PPC_TC))); >> >> -#define BPF_TAILCALL_PROLOGUE_SIZE 16 >> + EMIT(PPC_RAW_LI(bpf_to_ppc(BPF_REG_1) - 1, 0)); >> >> /* >> * We need a stack frame, but we don't necessarily need to >> @@ -244,7 +242,6 @@ static int bpf_jit_emit_tail_call(u32 *image, >> struct codegen_context *ctx, u32 o >> EMIT(PPC_RAW_RLWINM(_R3, b2p_index, 2, 0, 29)); >> EMIT(PPC_RAW_ADD(_R3, _R3, b2p_bpf_array)); >> EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_array, ptrs))); >> - EMIT(PPC_RAW_STW(_R0, _R1, bpf_jit_stack_offsetof(ctx, >> BPF_PPC_TC))); >> >> /* >> * if (prog == NULL) >> @@ -257,20 +254,20 @@ static int bpf_jit_emit_tail_call(u32 *image, >> struct codegen_context *ctx, u32 o >> EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_prog, bpf_func))); >> >> if (ctx->seen & SEEN_FUNC) >> - EMIT(PPC_RAW_LWZ(_R0, _R1, BPF_PPC_STACKFRAME(ctx) + >> PPC_LR_STKOFF)); >> + EMIT(PPC_RAW_LWZ(_R5, _R1, BPF_PPC_STACKFRAME(ctx) + >> PPC_LR_STKOFF)); >> >> EMIT(PPC_RAW_ADDIC(_R3, _R3, BPF_TAILCALL_PROLOGUE_SIZE)); >> >> if (ctx->seen & SEEN_FUNC) >> - EMIT(PPC_RAW_MTLR(_R0)); >> + EMIT(PPC_RAW_MTLR(_R5)); > > Should we explicitly zero-out _R5 after this? Don't know, is that required ? By the way if I start using _R5 instead of _R0 for TCC then this won't change. > > You can move the above PPC_RAW_LWZ() and PPC_RAW_MTLR() instructions, as > well as the ADDI below for r1 into bpf_jit_emit_common_epilogue() and > not have to repeat those here. Right. Allthough I wanted to minimise the churn. But yes I can do that especially as we'll now use _R5 for TCC and keep _R0 for mtlr. Christophe > > - Naveen > >> >> EMIT(PPC_RAW_MTCTR(_R3)); >> >> - EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_1))); >> - >> /* tear restore NVRs, ... */ >> bpf_jit_emit_common_epilogue(image, ctx); >> >> + EMIT(PPC_RAW_ADDI(_R1, _R1, BPF_PPC_STACKFRAME(ctx))); >> + >> EMIT(PPC_RAW_BCTR()); >> >> /* out: */ >> -- >> 2.38.1 >> >>
Christophe Leroy wrote: > > > Le 22/11/2022 à 08:33, Naveen N. Rao a écrit : >> Christophe Leroy wrote: >>> >>> This is a tentative to write above the stack. The problem is encoutered >>> with tests added by commit 38608ee7b690 ("bpf, tests: Add load store >>> test case for tail call") >>> >>> This happens because tail call is done to a BPF prog with a different >>> stack_depth. At the time being, the stack is kept as is when the caller >>> tail calls its callee. But at exit, the callee restores the stack based >>> on its own properties. Therefore here, at each run, r1 is erroneously >>> increased by 32 - 16 = 16 bytes. >>> >>> This was done that way in order to pass the tail call count from caller >>> to callee through the stack. As powerpc32 doesn't have a red zone in >>> the stack, it was necessary the maintain the stack as is for the tail >>> call. But it was not anticipated that the BPF frame size could be >>> different. >>> >>> Let's take a new approach. Use register r0 to carry the tail call count >>> during the tail call, and save it into the stack at function entry if >>> required. That's a deviation from the ppc32 ABI, but after all the way >>> tail calls are implemented is already not in accordance with the ABI. >> >> Can we pass the tail call count in r4 instead? > > It's a bit tricky. > > When entering the function through the normal entry point, the input > parameter is a 32 bits pointer and is in r3. > But at the begining of the function it gets moved to r4 and r3 is > cleared because it becomes a 64 bits parameter. > > When using the tailcall entry point, it is already in r4, and until now > r3 was containing garbage, with this patch r3 gets cleared as well. > > We could move the input pointer back into r3 for the tailcall as well, > but it would mean unnecessary register move. > > Or we can use r3 for the tailcall counter. Regarding zero'ing out r5, you're right -- we don't use the second parameter for a tail call, so that should be fine. Using r3 will be difficult since it is used when you come in first. My suggestion was something like the below (untested). Using r5 might be fine too. - Naveen -- diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c index 43f1c76d48cea9..6319283ce4ef01 100644 --- a/arch/powerpc/net/bpf_jit_comp32.c +++ b/arch/powerpc/net/bpf_jit_comp32.c @@ -113,24 +113,19 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx) { int i; + /* Initialize tail_call_cnt, to be skipped if we do tail calls. */ + EMIT(PPC_RAW_LI(_R4, 0)); + +#define BPF_TAILCALL_PROLOGUE_SIZE 4 + + if (ctx->seen & SEEN_TAILCALL) + EMIT(PPC_RAW_STW(_R4, _R1, bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); + /* First arg comes in as a 32 bits pointer. */ EMIT(PPC_RAW_MR(bpf_to_ppc(BPF_REG_1), _R3)); EMIT(PPC_RAW_LI(bpf_to_ppc(BPF_REG_1) - 1, 0)); EMIT(PPC_RAW_STWU(_R1, _R1, -BPF_PPC_STACKFRAME(ctx))); - /* - * Initialize tail_call_cnt in stack frame if we do tail calls. - * Otherwise, put in NOPs so that it can be skipped when we are - * invoked through a tail call. - */ - if (ctx->seen & SEEN_TAILCALL) - EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_1) - 1, _R1, - bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); - else - EMIT(PPC_RAW_NOP()); - -#define BPF_TAILCALL_PROLOGUE_SIZE 16 - /* * We need a stack frame, but we don't necessarily need to * save/restore LR unless we call other functions @@ -170,24 +165,24 @@ static void bpf_jit_emit_common_epilogue(u32 *image, struct codegen_context *ctx for (i = BPF_PPC_NVR_MIN; i <= 31; i++) if (bpf_is_seen_register(ctx, i)) EMIT(PPC_RAW_LWZ(i, _R1, bpf_jit_stack_offsetof(ctx, i))); -} - -void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx) -{ - EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_0))); - - bpf_jit_emit_common_epilogue(image, ctx); - - /* Tear down our stack frame */ if (ctx->seen & SEEN_FUNC) EMIT(PPC_RAW_LWZ(_R0, _R1, BPF_PPC_STACKFRAME(ctx) + PPC_LR_STKOFF)); + /* Tear down our stack frame */ EMIT(PPC_RAW_ADDI(_R1, _R1, BPF_PPC_STACKFRAME(ctx))); if (ctx->seen & SEEN_FUNC) EMIT(PPC_RAW_MTLR(_R0)); +} + +void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx) +{ + EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_0))); + + bpf_jit_emit_common_epilogue(image, ctx); + EMIT(PPC_RAW_BLR()); } @@ -244,7 +239,6 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o EMIT(PPC_RAW_RLWINM(_R3, b2p_index, 2, 0, 29)); EMIT(PPC_RAW_ADD(_R3, _R3, b2p_bpf_array)); EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_array, ptrs))); - EMIT(PPC_RAW_STW(_R0, _R1, bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); /* * if (prog == NULL) @@ -255,18 +249,11 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o /* goto *(prog->bpf_func + prologue_size); */ EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_prog, bpf_func))); - - if (ctx->seen & SEEN_FUNC) - EMIT(PPC_RAW_LWZ(_R0, _R1, BPF_PPC_STACKFRAME(ctx) + PPC_LR_STKOFF)); - EMIT(PPC_RAW_ADDIC(_R3, _R3, BPF_TAILCALL_PROLOGUE_SIZE)); - - if (ctx->seen & SEEN_FUNC) - EMIT(PPC_RAW_MTLR(_R0)); - EMIT(PPC_RAW_MTCTR(_R3)); EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_1))); + EMIT(PPC_RAW_MR(_R4, _R0)); /* tear restore NVRs, ... */ bpf_jit_emit_common_epilogue(image, ctx);
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c index 43f1c76d48ce..97e75b8181ca 100644 --- a/arch/powerpc/net/bpf_jit_comp32.c +++ b/arch/powerpc/net/bpf_jit_comp32.c @@ -115,21 +115,19 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx) /* First arg comes in as a 32 bits pointer. */ EMIT(PPC_RAW_MR(bpf_to_ppc(BPF_REG_1), _R3)); - EMIT(PPC_RAW_LI(bpf_to_ppc(BPF_REG_1) - 1, 0)); + EMIT(PPC_RAW_LI(_R0, 0)); + +#define BPF_TAILCALL_PROLOGUE_SIZE 8 + EMIT(PPC_RAW_STWU(_R1, _R1, -BPF_PPC_STACKFRAME(ctx))); /* - * Initialize tail_call_cnt in stack frame if we do tail calls. - * Otherwise, put in NOPs so that it can be skipped when we are - * invoked through a tail call. + * Save tail_call_cnt in stack frame if we do tail calls. */ if (ctx->seen & SEEN_TAILCALL) - EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_1) - 1, _R1, - bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); - else - EMIT(PPC_RAW_NOP()); + EMIT(PPC_RAW_STW(_R0, _R1, bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); -#define BPF_TAILCALL_PROLOGUE_SIZE 16 + EMIT(PPC_RAW_LI(bpf_to_ppc(BPF_REG_1) - 1, 0)); /* * We need a stack frame, but we don't necessarily need to @@ -244,7 +242,6 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o EMIT(PPC_RAW_RLWINM(_R3, b2p_index, 2, 0, 29)); EMIT(PPC_RAW_ADD(_R3, _R3, b2p_bpf_array)); EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_array, ptrs))); - EMIT(PPC_RAW_STW(_R0, _R1, bpf_jit_stack_offsetof(ctx, BPF_PPC_TC))); /* * if (prog == NULL) @@ -257,20 +254,20 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_prog, bpf_func))); if (ctx->seen & SEEN_FUNC) - EMIT(PPC_RAW_LWZ(_R0, _R1, BPF_PPC_STACKFRAME(ctx) + PPC_LR_STKOFF)); + EMIT(PPC_RAW_LWZ(_R5, _R1, BPF_PPC_STACKFRAME(ctx) + PPC_LR_STKOFF)); EMIT(PPC_RAW_ADDIC(_R3, _R3, BPF_TAILCALL_PROLOGUE_SIZE)); if (ctx->seen & SEEN_FUNC) - EMIT(PPC_RAW_MTLR(_R0)); + EMIT(PPC_RAW_MTLR(_R5)); EMIT(PPC_RAW_MTCTR(_R3)); - EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_1))); - /* tear restore NVRs, ... */ bpf_jit_emit_common_epilogue(image, ctx); + EMIT(PPC_RAW_ADDI(_R1, _R1, BPF_PPC_STACKFRAME(ctx))); + EMIT(PPC_RAW_BCTR()); /* out: */