On Thu, May 02, 2024 at 11:02:04PM GMT, Hari Bathini wrote:
> With PCREL addressing, there is no kernel TOC. So, it is not setup in
> prologue when PCREL addressing is used. But the number of instructions
> to skip on a tail call was not adjusted accordingly. That resulted in
> not so obvious failures while using tailcalls. 'tailcalls' selftest
> crashed the system with the below call trace:
>
> bpf_test_run+0xe8/0x3cc (unreliable)
> bpf_prog_test_run_skb+0x348/0x778
> __sys_bpf+0xb04/0x2b00
> sys_bpf+0x28/0x38
> system_call_exception+0x168/0x340
> system_call_vectored_common+0x15c/0x2ec
>
> Also, as bpf programs are always module addresses and a bpf helper in
> general is a core kernel text address, using PC relative addressing
> often fails with "out of range of pcrel address" error. Switch to
> using kernel base for relative addressing to handle this better.
>
> Fixes: 7e3a68be42e1 ("powerpc/64: vmlinux support building with PCREL addresing")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hari Bathini <hbathini@xxxxxxxxxxxxx>
> ---
>
> * Changes in v4:
> - Fix out of range errors by switching to kernelbase instead of PC
> for relative addressing.
>
> * Changes in v3:
> - New patch to fix tailcall issues with PCREL addressing.
>
>
> arch/powerpc/net/bpf_jit_comp64.c | 30 ++++++++++++++++--------------
> 1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 79f23974a320..4de08e35e284 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -202,7 +202,8 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
> EMIT(PPC_RAW_BLR());
> }
>
> -static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func)
> +static int
> +bpf_jit_emit_func_call_hlp(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
> {
> unsigned long func_addr = func ? ppc_function_entry((void *)func) : 0;
> long reladdr;
> @@ -211,19 +212,20 @@ static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u
> return -EINVAL;
>
> if (IS_ENABLED(CONFIG_PPC_KERNEL_PCREL)) {
> - reladdr = func_addr - CTX_NIA(ctx);
> + reladdr = func_addr - local_paca->kernelbase;
>
> if (reladdr >= (long)SZ_8G || reladdr < -(long)SZ_8G) {
> - pr_err("eBPF: address of %ps out of range of pcrel address.\n",
> - (void *)func);
> + pr_err("eBPF: address of %ps out of range of 34-bit relative address.\n",
> + (void *)func);
> return -ERANGE;
> }
> - /* pla r12,addr */
> - EMIT(PPC_PREFIX_MLS | __PPC_PRFX_R(1) | IMM_H18(reladdr));
> - EMIT(PPC_INST_PADDI | ___PPC_RT(_R12) | IMM_L(reladdr));
> - EMIT(PPC_RAW_MTCTR(_R12));
> - EMIT(PPC_RAW_BCTR());
> -
> + EMIT(PPC_RAW_LD(_R12, _R13, offsetof(struct paca_struct, kernelbase)));
> + /* Align for subsequent prefix instruction */
> + if (!IS_ALIGNED((unsigned long)fimage + CTX_NIA(ctx), 8))
> + EMIT(PPC_RAW_NOP());
We don't need the prefix instruction to be aligned to a doubleword
boundary - it just shouldn't cross a 64-byte boundary. Since we know the
exact address of the instruction here, we should be able to check for
that case.
> + /* paddi r12,r12,addr */
> + EMIT(PPC_PREFIX_MLS | __PPC_PRFX_R(0) | IMM_H18(reladdr));
> + EMIT(PPC_INST_PADDI | ___PPC_RT(_R12) | ___PPC_RA(_R12) | IMM_L(reladdr));
> } else {
> reladdr = func_addr - kernel_toc_addr();
> if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
> @@ -233,9 +235,9 @@ static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u
>
> EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
> EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
> - EMIT(PPC_RAW_MTCTR(_R12));
> - EMIT(PPC_RAW_BCTRL());
> }
> + EMIT(PPC_RAW_MTCTR(_R12));
> + EMIT(PPC_RAW_BCTRL());
This change shouldn't be necessary since these instructions are moved
back into the conditional in the next patch.
Other than those minor comments:
Reviewed-by: Naveen N Rao <naveen@xxxxxxxxxx>
- Naveen
