Re: [PATCH v4 1/2] powerpc64/bpf: fix tail calls for PCREL addressing

2024-05-08 Thread Michael Ellerman 
On Thu, 02 May 2024 23:02:04 +0530, 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
> 
> [...]

Applied to powerpc/next.

[1/2] powerpc64/bpf: fix tail calls for PCREL addressing
  https://git.kernel.org/powerpc/c/2ecfe59cd7de1f202e9af2516a61fbbf93d0bd4d
[2/2] powerpc/bpf: enable kfunc call
  https://git.kernel.org/powerpc/c/61688a82e047a4166436bf2665716cc070572ffa

cheers

Re: [PATCH v4 1/2] powerpc64/bpf: fix tail calls for PCREL addressing

2024-05-07 Thread Naveen N Rao 
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: sta...@vger.kernel.org
> Signed-off-by: Hari Bathini 
> ---
> 
> * 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 > 0x7FFF || reladdr < -(0x8000L)) {
> @@ -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

[PATCH v4 1/2] powerpc64/bpf: fix tail calls for PCREL addressing

2024-05-02 Thread Hari Bathini 
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: sta...@vger.kernel.org
Signed-off-by: Hari Bathini 
---

* 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());
+   /* 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 > 0x7FFF || reladdr < -(0x8000L)) {
@@ -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());
 
return 0;
 }
@@ -285,7 +287,7 @@ static int bpf_jit_emit_tail_call(u32 *image, struct 
codegen_context *ctx, u32 o
int b2p_index = bpf_to_ppc(BPF_REG_3);
int bpf_tailcall_prologue_size = 8;
 
-   if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
+   if (!IS_ENABLED(CONFIG_PPC_KERNEL_PCREL) && 
IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
bpf_tailcall_prologue_size += 4; /* skip past the toc load */
 
/*
@@ -993,7 +995,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 
*fimage, struct code
return ret;
 
if (func_addr_fixed)
-   ret = bpf_jit_emit_func_call_hlp(image, ctx, 
func_addr);
+   ret = bpf_jit_emit_func_call_hlp(image, fimage, 
ctx, func_addr);
else
ret = bpf_jit_emit_func_call_rel(image, fimage, 
ctx, func_addr);
 
-- 
2.44.0