On 19/04/25 4:31 pm, Naveen N Rao wrote:
On Thu, Apr 17, 2025 at 01:10:37AM +0530, Hari Bathini wrote:
arch_bpf_trampoline_size() provides JIT size of the BPF trampoline
before the buffer for JIT'ing it is allocated. The total number of
instructions emitted for BPF trampoline JIT code depends on where
the final image is located. So, the size arrived at with the dummy
pass in arch_bpf_trampoline_size() can vary from the actual size
needed in  arch_prepare_bpf_trampoline().  When the instructions
accounted in  arch_bpf_trampoline_size() is less than the number of
instructions emitted during the actual JIT compile of the trampoline,
the below warning is produced:

   WARNING: CPU: 8 PID: 204190 at arch/powerpc/net/bpf_jit_comp.c:981 
__arch_prepare_bpf_trampoline.isra.0+0xd2c/0xdcc

which is:

   /* Make sure the trampoline generation logic doesn't overflow */
   if (image && WARN_ON_ONCE(&image[ctx->idx] >
                        (u32 *)rw_image_end - BPF_INSN_SAFETY)) {

So, during the dummy pass, instead of providing some arbitrary image
location, account for maximum possible instructions if and when there
is a dependency with image location for JIT'ing.

Fixes: d243b62b7bd3 ("powerpc64/bpf: Add support for bpf trampolines")
Reported-by: Venkat Rao Bagalkote <venka...@linux.ibm.com>
Closes: 
https://lore.kernel.org/all/6168bfc8-659f-4b5a-a6fb-90a916dde...@linux.ibm.com/
Cc: sta...@vger.kernel.org # v6.13+
Signed-off-by: Hari Bathini <hbath...@linux.ibm.com>
---

Changes since v1:
- Pass NULL for image during intial pass and account for max. possible
   instruction during this pass as Naveen suggested.


  arch/powerpc/net/bpf_jit.h        | 20 ++++++++++++++++---
  arch/powerpc/net/bpf_jit_comp.c   | 33 ++++++++++---------------------
  arch/powerpc/net/bpf_jit_comp64.c |  9 +++++++++
  3 files changed, 36 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index 6beacaec63d3..4c26912c2e3c 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -51,8 +51,16 @@
                EMIT(PPC_INST_BRANCH_COND | (((cond) & 0x3ff) << 16) | (offset 
& 0xfffc));                                        \
        } while (0)
-/* Sign-extended 32-bit immediate load */
+/*
+ * Sign-extended 32-bit immediate load
+ *
+ * If this is a dummy pass (!image), account for
+ * maximum possible instructions.
+ */
  #define PPC_LI32(d, i)                do {                                    
      \
+       if (!image)                                                           \
+               ctx->idx += 2;                                                  
   \
+       else {                                                                \
                if ((int)(uintptr_t)(i) >= -32768 &&                       \
                                (int)(uintptr_t)(i) < 32768)               \
                        EMIT(PPC_RAW_LI(d, i));                               \
@@ -60,10 +68,15 @@
                        EMIT(PPC_RAW_LIS(d, IMM_H(i)));                       \
                        if (IMM_L(i))                                         \
                                EMIT(PPC_RAW_ORI(d, d, IMM_L(i)));            \
-               } } while(0)
+               }                                                             \
+       } } while (0)
#ifdef CONFIG_PPC64
+/* If dummy pass (!image), account for maximum possible instructions */
  #define PPC_LI64(d, i)                do {                                    
      \
+       if (!image)                                                           \
+               ctx->idx += 5;                                                  
   \
+       else {                                                                \
                if ((long)(i) >= -2147483648 &&                                 
   \
                                (long)(i) < 2147483648)                         
   \
                        PPC_LI32(d, i);                                       \
@@ -84,7 +97,8 @@
                        if ((uintptr_t)(i) & 0x000000000000ffffULL)       \
                                EMIT(PPC_RAW_ORI(d, d, (uintptr_t)(i) &       \
                                                        0xffff));             \
-               } } while (0)
+               }                                                             \
+       } } while (0)

You should now also be able to remove the padding we add in
bpf_jit_comp64.c for 'case BPF_LD | BPF_IMM | BPF_DW:'

  #define PPC_LI_ADDR   PPC_LI64
#ifndef CONFIG_PPC_KERNEL_PCREL
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 2991bb171a9b..c0684733e9d6 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -504,10 +504,11 @@ static int invoke_bpf_prog(u32 *image, u32 *ro_image, 
struct codegen_context *ct
        EMIT(PPC_RAW_ADDI(_R3, _R1, regs_off));
        if (!p->jited)
                PPC_LI_ADDR(_R4, (unsigned long)p->insnsi);
-       if (!create_branch(&branch_insn, (u32 *)&ro_image[ctx->idx], (unsigned 
long)p->bpf_func,
-                          BRANCH_SET_LINK)) {
-               if (image)
-                       image[ctx->idx] = ppc_inst_val(branch_insn);
+       /* Account for max possible instructions during dummy pass for size 
calculation */
+       if (image && !create_branch(&branch_insn, (u32 *)&ro_image[ctx->idx],
+                                   (unsigned long)p->bpf_func,
+                                   BRANCH_SET_LINK)) {
+               image[ctx->idx] = ppc_inst_val(branch_insn);
                ctx->idx++;
        } else {
                EMIT(PPC_RAW_LL(_R12, _R25, offsetof(struct bpf_prog, 
bpf_func)));
@@ -889,7 +890,8 @@ static int __arch_prepare_bpf_trampoline(struct 
bpf_tramp_image *im, void *rw_im
                        bpf_trampoline_restore_tail_call_cnt(image, ctx, 
func_frame_offset, r4_off);
/* Reserve space to patch branch instruction to skip fexit progs */
-               im->ip_after_call = &((u32 *)ro_image)[ctx->idx];
+               if (ro_image) /* image is NULL for dummy pass */
+                       im->ip_after_call = &((u32 *)ro_image)[ctx->idx];
                EMIT(PPC_RAW_NOP());
        }
@@ -912,7 +914,8 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *rw_im
                }
if (flags & BPF_TRAMP_F_CALL_ORIG) {
-               im->ip_epilogue = &((u32 *)ro_image)[ctx->idx];
+               if (ro_image) /* image is NULL for dummy pass */
+                       im->ip_epilogue = &((u32 *)ro_image)[ctx->idx];
                PPC_LI_ADDR(_R3, im);
                ret = bpf_jit_emit_func_call_rel(image, ro_image, ctx,
                                                 (unsigned 
long)__bpf_tramp_exit);
@@ -973,25 +976,9 @@ int arch_bpf_trampoline_size(const struct btf_func_model 
*m, u32 flags,
                             struct bpf_tramp_links *tlinks, void *func_addr)
  {
        struct bpf_tramp_image im;
-       void *image;
        int ret;
- /*
-        * Allocate a temporary buffer for __arch_prepare_bpf_trampoline().
-        * This will NOT cause fragmentation in direct map, as we do not
-        * call set_memory_*() on this buffer.
-        *
-        * We cannot use kvmalloc here, because we need image to be in
-        * module memory range.
-        */
-       image = bpf_jit_alloc_exec(PAGE_SIZE);
-       if (!image)
-               return -ENOMEM;
-
-       ret = __arch_prepare_bpf_trampoline(&im, image, image + PAGE_SIZE, 
image,
-                                           m, flags, tlinks, func_addr);
-       bpf_jit_free_exec(image);
-
+       ret = __arch_prepare_bpf_trampoline(&im, NULL, NULL, NULL, m, flags, 
tlinks, func_addr);
        return ret;
  }
diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index 233703b06d7c..91f9efe8b8d7 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -225,6 +225,15 @@ int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, 
struct codegen_context *
        }
#ifdef CONFIG_PPC_KERNEL_PCREL
+       /*
+        * If fimage is NULL (the initial pass to find image size),
+        * account for the maximum no. of instructions possible.
+        */
+       if (!fimage) {
+               ctx->idx += 7;
+               return 0;
+       }
+

I would merge this with the below if conditional so that this gets
noticed if the instruction sequence below ever changes.

        reladdr = func_addr - local_paca->kernelbase;
if (reladdr < (long)SZ_8G && reladdr >= -(long)SZ_8G) {

Other than that:
Acked-by: Naveen N Rao (AMD) <nav...@kernel.org>

Thanks for the review, Naveen.
Posted v3 with the suggested change.

https://lore.kernel.org/linuxppc-dev/20250422082609.949301-1-hbath...@linux.ibm.com/

- Hari

Reply via email to