powerpc/bpf: fix alignment of long branch trampoline address
Subsystems (6)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v11 1/8 powerpc/bpf: fix alignment of long branch trampoline address
From: Abhishek Dubey <[email protected]>
Ensure the dummy trampoline address field present between the OOL stub and the long branch stub is 8-byte aligned, for memory compatibility when content loaded to a register.
--- arch/powerpc/net/bpf_jit.h | 7 +++--- arch/powerpc/net/bpf_jit_comp.c | 42 ++++++++++++++++++++++++++----- arch/powerpc/net/bpf_jit_comp32.c | 6 +++-- arch/powerpc/net/bpf_jit_comp64.c | 7 +++--- 4 files changed, 48 insertions(+), 14 deletions(-)
arch/powerpc/net/bpf_jit.h +4 -3
@@ -214,10 +214,11 @@ int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *int bpf_jit_build_body (struct bpf_prog *fp,u32 *image,u32 *fimage,struct codegen_context *ctx,u32 *addrs,int pass,bool extra_pass);void bpf_jit_build_prologue (u32 *image,struct codegen_context *ctx); -void bpf_jit_build_epilogue (u32 *image,struct codegen_context *ctx); -void bpf_jit_build_fentry_stubs (u32 *image,struct codegen_context *ctx); +void bpf_jit_build_epilogue (u32 *image,u32 *fimage,struct codegen_context *ctx); +void bpf_jit_build_fentry_stubs (u32 *image,u32 *fimage,struct codegen_context *ctx);void bpf_jit_realloc_regs (struct codegen_context *ctx); -int bpf_jit_emit_exit_insn (u32 *image,struct codegen_context *ctx,int tmp_reg,long exit_addr); +int bpf_jit_emit_exit_insn (u32 *image,u32 *fimage,struct codegen_context *ctx,int tmp_reg, +long exit_addr);void prepare_for_fsession_fentry (u32 *image,struct codegen_context *ctx,int cookie_cnt,int cookie_off,int retval_off);void store_func_meta (u32 *image,struct codegen_context *ctx,u64 func_meta,int func_meta_off);
arch/powerpc/net/bpf_jit_comp.c +36 -6
88 lines not on this page · raw message
arch/powerpc/net/bpf_jit_comp32.c +4 -2
@@ -229,7 +229,7 @@ static void bpf_jit_emit_common_epilogue(u32 *image, struct codegen_context *ctx } -void bpf_jit_build_epilogue (u32 *image,struct codegen_context *ctx) +void bpf_jit_build_epilogue (u32 *image,u32 *fimage,struct codegen_context *ctx) {EMIT (PPC_RAW_MR (_R3,bpf_to_ppc (BPF_REG_0 )));
@@ -237,7 +237,7 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)EMIT (PPC_RAW_BLR ()); -bpf_jit_build_fentry_stubs (image, ctx); +bpf_jit_build_fentry_stubs (image, fimage, ctx); }/* Relative offset needs to be calculated based on final image location */
@@ -1150,6 +1150,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code*/ if (i != flen -1 ) { ret =bpf_jit_emit_exit_insn (image, ctx, _R0, exit_addr); + ret =bpf_jit_emit_exit_insn (image, fimage, + ctx, _R0, exit_addr);if (ret)return ret; }
arch/powerpc/net/bpf_jit_comp64.c +4 -3
@@ -398,7 +398,7 @@ static void bpf_jit_emit_common_epilogue(u32 *image, struct codegen_context *ctx } } -void bpf_jit_build_epilogue (u32 *image,struct codegen_context *ctx) +void bpf_jit_build_epilogue (u32 *image,u32 *fimage,struct codegen_context *ctx) {bpf_jit_emit_common_epilogue (image, ctx);
@@ -407,7 +407,7 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)EMIT (PPC_RAW_BLR ()); -bpf_jit_build_fentry_stubs (image, ctx); +bpf_jit_build_fentry_stubs (image, fimage, ctx); }/*
@@ -1737,7 +1737,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code* we'll just fall through to the epilogue. */ if (i != flen -1 ) { - ret =bpf_jit_emit_exit_insn (image, ctx, tmp1_reg, exit_addr); + ret =bpf_jit_emit_exit_insn (image, fimage, ctx, + tmp1_reg, exit_addr);if (ret)return ret; }
Signature · 1 line
2.54.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Dynamic epilogue sizing breaks JIT pass convergence, causing BPF_EXIT branches to skip the first instruction of the epilogue.
- [Low] The patch introduces a compilation error in arch/powerpc/net/bpf_jit_comp32.c by failing to remove the old 4-argument call to bpf_jit_emit_exit_insn().
Pre-existing issues:
- [Critical] Out-of-bounds write in BPF JIT due to under-calculated allocation size for large programs. --
--- Patch [1]: [PATCH v11 1/8] powerpc/bpf: fix alignment of long branch trampoline address --- commit a15c5c8b15ef787442dd6c60c0e42163a958cb5a
Author: Abhishek Dubey <[email protected]>
powerpc/bpf: fix alignment of long branch trampoline address
This patch ensures the dummy trampoline address field between the out-of-line stub and the long branch stub is 8-byte aligned. It modifies the epilogue and fentry stub building functions to accept the final image pointer and emit a conditional NOP if needed.
Quoted 4 lines
arch/powerpc/net/bpf_jit_comp.c +0
-0
[ ... ]
Quoted 26 lines
arch/powerpc/net/bpf_jit_comp.c +17
-1@@ -49,11 +49,39 @@ asm (" .popsection ;" ); -void bpf_jit_build_fentry_stubs (u32 *image,struct codegen_context *ctx) +void bpf_jit_build_fentry_stubs (u32 *image,u32 *fimage,struct codegen_context *ctx) {int ool_stub_idx, long_branch_stub_idx; +int ool_stub_sz;/* +* In the final pass, align the mis-aligned dummy_tramp_addr field +* in the fimage. The alignment NOP must appear before OOL stub, +* to make ool_stub_idx & long_branch_stub_idx constant from end. +* +* dummy_tramp_addr must be 8-byte aligned for load-register +* compatibility. The fimage can be non 8-byte aligned, so final +* alignment depends on start of fimage and the stub's instruction +* count offset. The OOL stub size is 4 instructions (with +* CONFIG_PPC_FTRACE_OUT_OF_LINE) or 3 instructions (without) +* before dummy_tramp_addr. +* +* Emit a NOP here if (ctx->idx + ool_stub_sz) is odd, so that +* dummy_tramp_addr lands at an even instruction offset (== 8-byte +* aligned from an 8-byte aligned base). +*
arch/powerpc/net/bpf_jit_comp.c asm ( · line 82
Quoted 10 lines
arch/powerpc/net/bpf_jit_comp.c +10
-0+* In pass=0 when image==NULL, conservatively account for space +* required to accommodate alignment NOP. In case final pass skips +* emitting alignment NOP, the image buffer have 4 spare bytes and +* jited_len signifies correct program size. +*/ + + ool_stub_sz =IS_ENABLED (CONFIG_PPC_FTRACE_OUT_OF_LINE ) ?16 :12 ; +if (!image || !IS_ALIGNED ((unsigned long )fimage + ctx->idx*4 + ool_stub_sz,SZL )) +EMIT (PPC_RAW_NOP ()); +
[Severity: High]
Does this conditional NOP break the JIT convergence loop in bpf_int_jit_compile()?
If the body size shrinks by an odd number of instructions in the final JIT pass, this could flip the alignment and cause an extra padding NOP to be emitted.
This would increase the epilogue size by 4 bytes while the total proglen remains constant. Could this cause the JIT loop to terminate prematurely while exit_addr still points to the old epilogue start, skipping the first instruction of the epilogue and potentially leading to register corruption?
Quoted 2 lines
arch/powerpc/net/bpf_jit_comp.c +0
-1/* -* nop // optional, for alignment of dummy_tramp_addr
[ ... ]
arch/powerpc/net/bpf_jit_comp.c void bpf_jit_build_fentry_stubs(u32 *image, struct codegen_context *ctx) · line 132
Quoted 9 lines
arch/powerpc/net/bpf_jit_comp.c +2
-1@@ -97,7 +126,8 @@ void bpf_jit_build_fentry_stubs(u32 *image, struct codegen_context *ctx) } } -int bpf_jit_emit_exit_insn (u32 *image,struct codegen_context *ctx,int tmp_reg,long exit_addr) +int bpf_jit_emit_exit_insn (u32 *image,u32 *fimage,struct codegen_context *ctx, +int tmp_reg,long exit_addr) {if (!exit_addr ||is_offset_in_branch_range (exit_addr - (ctx->idx *4 ))) {
[Severity: Critical]
This is a pre-existing issue, but does the !exit_addr condition falsely report large branches as in-range during Pass 0?
In bpf_int_jit_compile(), the true epilogue offset is populated after the second Pass 0 loop, meaning during all Pass 0 execution, exit_addr is evaluated as 0. This bypasses the branch range evaluation and falsely predicts only 4 bytes per BPF_EXIT.
When exit_addr is resolved in Pass 1 for large programs (>32MB) that exceed the branch range, could this cause an under-calculated allocation size, leading to an out-of-bounds write when the inline epilogue is emitted?
Quoted 1 line
arch/powerpc/net/bpf_jit_comp.c +0
-0PPC_JMP (exit_addr);
[ ... ]
Quoted 4 lines
arch/powerpc/net/bpf_jit_comp32.c +0
-0
[ ... ]
arch/powerpc/net/bpf_jit_comp32.c int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code · line 1157
Quoted 9 lines
arch/powerpc/net/bpf_jit_comp32.c +2
-0@@ -1150,6 +1150,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code*/ if (i != flen -1 ) { ret =bpf_jit_emit_exit_insn (image, ctx, _R0, exit_addr); + ret =bpf_jit_emit_exit_insn (image, fimage, + ctx, _R0, exit_addr);if (ret)return ret; }
[Severity: Low]
Is the old 4-argument call to bpf_jit_emit_exit_insn() accidentally left here?
Leaving this intact right above the newly added 5-argument call appears to directly break the build on 32-bit PowerPC architectures due to too few arguments.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Hari Bathini
maintainerQuoted 160 lines · Saket Kumar Bhaskar
From: Abhishek Dubey <[email protected]>
Ensure the dummy trampoline address field present between the OOL stub and the long branch stub is 8-byte aligned, for memory compatibility when content loaded to a register.
Reported-by Hari Bathini <[email protected]>Fixes d243b62b7bd3 ("powerpc64/bpf: Add support for bpf trampolines")Signed-off-by Abhishek Dubey <[email protected]>Signed-off-by Saket Kumar Bhaskar <[email protected]>--- arch/powerpc/net/bpf_jit.h | 7 +++--- arch/powerpc/net/bpf_jit_comp.c | 42 ++++++++++++++++++++++++++----- arch/powerpc/net/bpf_jit_comp32.c | 6 +++-- arch/powerpc/net/bpf_jit_comp64.c | 7 +++--- 4 files changed, 48 insertions(+), 14 deletions(-)arch/powerpc/net/bpf_jit.h +4
-3@@ -214,10 +214,11 @@ int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *int bpf_jit_build_body (struct bpf_prog *fp,u32 *image,u32 *fimage,struct codegen_context *ctx,u32 *addrs,int pass,bool extra_pass);void bpf_jit_build_prologue (u32 *image,struct codegen_context *ctx); -void bpf_jit_build_epilogue (u32 *image,struct codegen_context *ctx); -void bpf_jit_build_fentry_stubs (u32 *image,struct codegen_context *ctx); +void bpf_jit_build_epilogue (u32 *image,u32 *fimage,struct codegen_context *ctx); +void bpf_jit_build_fentry_stubs (u32 *image,u32 *fimage,struct codegen_context *ctx);void bpf_jit_realloc_regs (struct codegen_context *ctx); -int bpf_jit_emit_exit_insn (u32 *image,struct codegen_context *ctx,int tmp_reg,long exit_addr); +int bpf_jit_emit_exit_insn (u32 *image,u32 *fimage,struct codegen_context *ctx,int tmp_reg, +long exit_addr);void prepare_for_fsession_fentry (u32 *image,struct codegen_context *ctx,int cookie_cnt,int cookie_off,int retval_off);void store_func_meta (u32 *image,struct codegen_context *ctx,u64 func_meta,int func_meta_off);arch/powerpc/net/bpf_jit_comp.c +36
-6@@ -49,11 +49,39 @@ asm (" .popsection ;" ); -void bpf_jit_build_fentry_stubs (u32 *image,struct codegen_context *ctx) +void bpf_jit_build_fentry_stubs (u32 *image,u32 *fimage,struct codegen_context *ctx) {int ool_stub_idx, long_branch_stub_idx; +int ool_stub_sz;/* +* In the final pass, align the mis-aligned dummy_tramp_addr field +* in the fimage. The alignment NOP must appear before OOL stub, +* to make ool_stub_idx & long_branch_stub_idx constant from end. +* +* dummy_tramp_addr must be 8-byte aligned for load-register +* compatibility. The fimage can be non 8-byte aligned, so final +* alignment depends on start of fimage and the stub's instruction +* count offset. The OOL stub size is 4 instructions (with +* CONFIG_PPC_FTRACE_OUT_OF_LINE) or 3 instructions (without) +* before dummy_tramp_addr. +* +* Emit a NOP here if (ctx->idx + ool_stub_sz) is odd, so that +* dummy_tramp_addr lands at an even instruction offset (== 8-byte +* aligned from an 8-byte aligned base). +* +* In pass=0 when image==NULL, conservatively account for space +* required to accommodate alignment NOP. In case final pass skips +* emitting alignment NOP, the image buffer have 4 spare bytes and +* jited_len signifies correct program size. +*/ + + ool_stub_sz =IS_ENABLED (CONFIG_PPC_FTRACE_OUT_OF_LINE ) ?16 :12 ; +if (!image || !IS_ALIGNED ((unsigned long )fimage + ctx->idx*4 + ool_stub_sz,SZL )) +EMIT (PPC_RAW_NOP ()); + +/* +* nop // optional, for alignment of dummy_tramp_addr * Out-of-line stub: * mflr r0 * [b|bl] tramp @@ -70,7 +98,7 @@ void bpf_jit_build_fentry_stubs(u32 *image, struct codegen_context *ctx)/* * Long branch stub: -* .long <dummy_tramp_addr> +* .long <dummy_tramp_addr> // 8-byte aligned * mflr r11 * bcl 20,31,$+4 * mflr r12 @@ -81,6 +109,7 @@ void bpf_jit_build_fentry_stubs(u32 *image, struct codegen_context *ctx)*/ if (image) *((unsigned long *)&image[ctx->idx]) = (unsigned long )dummy_tramp; + ctx->idx +=SZL /4 ; long_branch_stub_idx = ctx->idx;EMIT (PPC_RAW_MFLR (_R11));@@ -97,7 +126,8 @@ void bpf_jit_build_fentry_stubs(u32 *image, struct codegen_context *ctx) } } -int bpf_jit_emit_exit_insn (u32 *image,struct codegen_context *ctx,int tmp_reg,long exit_addr) +int bpf_jit_emit_exit_insn (u32 *image,u32 *fimage,struct codegen_context *ctx, +int tmp_reg,long exit_addr) {if (!exit_addr ||is_offset_in_branch_range (exit_addr - (ctx->idx *4 ))) {PPC_JMP (exit_addr);@@ -107,7 +137,7 @@ int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg,PPC_JMP (ctx->alt_exit_addr); }else { ctx->alt_exit_addr = ctx->idx *4 ; -bpf_jit_build_epilogue (image, ctx); +bpf_jit_build_epilogue (image, fimage, ctx); }return 0 ;@@ -286,7 +316,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr*/ bpf_jit_build_prologue (NULL , &cgctx); addrs[fp->len] = cgctx.idx *4 ; -bpf_jit_build_epilogue (NULL , &cgctx); +bpf_jit_build_epilogue (NULL ,NULL , &cgctx); fixup_len = fp->aux->num_exentries *BPF_FIXUP_LEN *4 ; extable_len = fp->aux->num_exentries *sizeof (struct exception_table_entry );@@ -318,7 +348,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_prbpf_jit_binary_pack_free (fhdr, hdr); goto out_err; } -bpf_jit_build_epilogue (code_base, &cgctx); +bpf_jit_build_epilogue (code_base, fcode_base, &cgctx);if (bpf_jit_enable >1 ) pr_info("Pass %d: shrink = %d, seen = 0x%x\n", pass,arch/powerpc/net/bpf_jit_comp32.c +2
-2@@ -229,7 +229,7 @@ static void bpf_jit_emit_common_epilogue(u32 *image, struct codegen_context *ctx } -void bpf_jit_build_epilogue (u32 *image,struct codegen_context *ctx) +void bpf_jit_build_epilogue (u32 *image,u32 *fimage,struct codegen_context *ctx) {EMIT (PPC_RAW_MR (_R3,bpf_to_ppc (BPF_REG_0 )));@@ -237,7 +237,7 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)EMIT (PPC_RAW_BLR ()); -bpf_jit_build_fentry_stubs (image, ctx); +bpf_jit_build_fentry_stubs (image, fimage, ctx); }/* Relative offset needs to be calculated based on final image location */
arch/powerpc/net/bpf_jit_comp32.c int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code · line 1153
Quoted 4 lines
arch/powerpc/net/bpf_jit_comp32.c +0
-0@@ -1150,6 +1150,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code*/ if (i != flen -1 ) { ret =bpf_jit_emit_exit_insn (image, ctx, _R0, exit_addr);
This breaks build on ppc32.
Quoted 28 lines
arch/powerpc/net/bpf_jit_comp32.c +2
-0+ ret =bpf_jit_emit_exit_insn (image, fimage, + ctx, _R0, exit_addr);if (ret)return ret;}arch/powerpc/net/bpf_jit_comp64.c +2
-2@@ -398,7 +398,7 @@ static void bpf_jit_emit_common_epilogue(u32 *image, struct codegen_context *ctx } } -void bpf_jit_build_epilogue (u32 *image,struct codegen_context *ctx) +void bpf_jit_build_epilogue (u32 *image,u32 *fimage,struct codegen_context *ctx) {bpf_jit_emit_common_epilogue (image, ctx);@@ -407,7 +407,7 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)EMIT (PPC_RAW_BLR ()); -bpf_jit_build_fentry_stubs (image, ctx); +bpf_jit_build_fentry_stubs (image, fimage, ctx); }/*
arch/powerpc/net/bpf_jit_comp64.c int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code · line 1744
Quoted 9 lines
arch/powerpc/net/bpf_jit_comp64.c +2
-1@@ -1737,7 +1737,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code* we'll just fall through to the epilogue. */ if (i != flen -1 ) { - ret =bpf_jit_emit_exit_insn (image, ctx, tmp1_reg, exit_addr); + ret =bpf_jit_emit_exit_insn (image, fimage, ctx, + tmp1_reg, exit_addr);if (ret)return ret; }
Have a general comment on the order of the patches. Can we have all powerpc specific patches first and the selftest changes at the end. The patch order should be patch #7 followed by patches #1 & #2, then patch #5 & patch #8 while patches #3, #4 & #6 are kept at the end. Makes for cleaner backporting given the many fixes tags..
- Hari
PATCH v11 2/8 powerpc/bpf: Move out dummy_tramp_addr after Long branch stub
In reply to a message not in the archiveFrom: Abhishek Dubey <[email protected]>
Move the long branch address field to the bottom of the long branch stub. This allows uninterrupted disassembly until the last 4/8 bytes. The last bytes exclusion is logically necessary to prevent disassembly failure, otherwise the actual program layout is never altered. Hence no effect on overall program size. Also, align dummy_tramp_addr field with 8-byte boundary.
Following is disassembler output for test program with moved down dummy_tramp_addr field:
.....
.....
pc:68 left:44 a6 03 08 7c : mtlr 0 pc:72 left:40 bc ff ff 4b : b .-68 pc:76 left:36 a6 02 68 7d : mflr 11 pc:80 left:32 05 00 9f 42 : bcl 20, 31, .+4 pc:84 left:28 a6 02 88 7d : mflr 12 pc:88 left:24 14 00 8c e9 : ld 12, 20(12) pc:92 left:20 a6 03 89 7d : mtctr 12 pc:96 left:16 a6 03 68 7d : mtlr 11 pc:100 left:12 20 04 80 4e : bctr pc:104 left:8 c0 34 1d 00 :
Failure log:
Can't disasm instruction at offset 104: c0 34 1d 00 00 00 00 c0 Disassembly logic can truncate at 104, ignoring last 8 bytes.
Update the dummy_tramp_addr field offset calculation from the end of the program to reflect its new location, for bpf_arch_text_poke() to update the actual trampoline's address in this field.
All BPF trampoline selftests continue to pass with this patch applied.
--- arch/powerpc/net/bpf_jit_comp.c | 46 +++++++++++++++++---------------- 1 file changed, 24 insertions(+), 22 deletions(-)
arch/powerpc/net/bpf_jit_comp.c +24 -22
@@ -52,9 +52,10 @@ asm (void bpf_jit_build_fentry_stubs (u32 *image,u32 *fimage,struct codegen_context *ctx) {int ool_stub_idx, long_branch_stub_idx; -int ool_stub_sz; +int stub_sz;/* +* The dummy_tramp_addr field is placed at bottom of Long branch stub. * In the final pass, align the mis-aligned dummy_tramp_addr field * in the fimage. The alignment NOP must appear before OOL stub, * to make ool_stub_idx & long_branch_stub_idx constant from end.
@@ -62,13 +63,10 @@ void bpf_jit_build_fentry_stubs(u32 *image, u32 *fimage, struct codegen_context* dummy_tramp_addr must be 8-byte aligned for load-register * compatibility. The fimage can be non 8-byte aligned, so final * alignment depends on start of fimage and the stub's instruction -* count offset. The OOL stub size is 4 instructions (with -* CONFIG_PPC_FTRACE_OUT_OF_LINE) or 3 instructions (without) -* before dummy_tramp_addr. -* -* Emit a NOP here if (ctx->idx + ool_stub_sz) is odd, so that -* dummy_tramp_addr lands at an even instruction offset (== 8-byte -* aligned from an 8-byte aligned base). +* count. The stubs block has 11 instructions (with +* CONFIG_PPC_FTRACE_OUT_OF_LINE) or 10 instructions (without) +* before dummy_tramp_addr field. Emit a NOP if the address of +* dummy_tramp_addr is non aligned. * * In pass=0 when image==NULL, conservatively account for space * required to accommodate alignment NOP. In case final pass skips
@@ -76,8 +74,8 @@ void bpf_jit_build_fentry_stubs(u32 *image, u32 *fimage, struct codegen_context* jited_len signifies correct program size. */ - ool_stub_sz =IS_ENABLED (CONFIG_PPC_FTRACE_OUT_OF_LINE ) ?16 :12 ; -if (!image || !IS_ALIGNED ((unsigned long )fimage + ctx->idx*4 + ool_stub_sz,SZL )) + stub_sz =IS_ENABLED (CONFIG_PPC_FTRACE_OUT_OF_LINE ) ?44 :40 ; +if (!image || !IS_ALIGNED ((unsigned long )fimage + ctx->idx*4 + stub_sz,SZL ))EMIT (PPC_RAW_NOP ());/*
@@ -98,28 +96,29 @@ void bpf_jit_build_fentry_stubs(u32 *image, u32 *fimage, struct codegen_context/* * Long branch stub: -* .long <dummy_tramp_addr> // 8-byte aligned * mflr r11 * bcl 20,31,$+4 -* mflr r12 -* ld r12, -8-SZL(r12) +* mflr r12 // lr/r12 stores pc of current(this) inst. +* ld r12, 20(r12) // offset(dummy_tramp_addr) from prev inst. is 20 * mtctr r12 -* mtlr r11 // needed to retain ftrace ABI +* mtlr r11 // needed to retain ftrace ABI * bctr +* .long <dummy_tramp_addr> // SZL bytes aligned */ -if (image) - *((unsigned long *)&image[ctx->idx]) = (unsigned long )dummy_tramp; - - ctx->idx +=SZL /4 ; long_branch_stub_idx = ctx->idx;EMIT (PPC_RAW_MFLR (_R11));EMIT (PPC_RAW_BCL4 ());EMIT (PPC_RAW_MFLR (_R12)); -EMIT (PPC_RAW_LL (_R12, _R12,-8 -SZL )); +EMIT (PPC_RAW_LL (_R12, _R12,20 ));EMIT (PPC_RAW_MTCTR (_R12));EMIT (PPC_RAW_MTLR (_R11));EMIT (PPC_RAW_BCTR ()); +if (image) + *((unsigned long *)&image[ctx->idx]) = (unsigned long )dummy_tramp; + + ctx->idx +=SZL /4 ; +if (!bpf_jit_ool_stub) { bpf_jit_ool_stub = (ctx->idx - ool_stub_idx) *4 ; bpf_jit_long_branch_stub = (ctx->idx - long_branch_stub_idx) *4 ;
@@ -1295,6 +1294,7 @@ static void do_isync(void *info __maybe_unused)* bpf_func: * [nop|b] ool_stub * 2. Out-of-line stub: +* nop // optional nop for alignment * ool_stub: * mflr r0 * [b|bl] <bpf_prog>/<long_branch_stub>
@@ -1302,14 +1302,14 @@ static void do_isync(void *info __maybe_unused)* b bpf_func + 4 * 3. Long branch stub: * long_branch_stub: -* .long <branch_addr>/<dummy_tramp> * mflr r11 * bcl 20,31,$+4 * mflr r12 -* ld r12, -16(r12) +* ld r12, 20(r12) * mtctr r12 * mtlr r11 // needed to retain ftrace ABI * bctr +* .long <branch_addr>/<dummy_tramp> * * dummy_tramp is used to reduce synchronization requirements. *
@@ -1411,10 +1411,12 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,* 1. Update the address in the long branch stub: * If new_addr is out of range, we will have to use the long branch stub, so patch new_addr * here. Otherwise, revert to dummy_tramp, but only if we had patched old_addr here. +* +* dummy_tramp_addr moved to bottom of long branch stub. */ if ((new_addr && !is_offset_in_branch_range (new_addr - ip)) || (old_addr && !is_offset_in_branch_range (old_addr - ip))) - ret =patch_ulong ((void *)(bpf_func_end - bpf_jit_long_branch_stub -SZL ), + ret =patch_ulong ((void *)(bpf_func_end -SZL ),/* SZL: dummy_tramp_addr offset */ (new_addr && !is_offset_in_branch_range (new_addr - ip)) ? (unsigned long )new_addr : (unsigned long )dummy_tramp);if (ret)
Signature · 1 line
2.54.0
PATCH v11 3/8 selftests/bpf: Fixing powerpc JIT disassembly failure
In reply to a message not in the archiveFrom: Abhishek Dubey <[email protected]>
Ensure that the trampoline stubs JITed at the tail of the epilogue do not expose the dummy trampoline address stored in the last 8 bytes(64-bit) and last 4 bytes(32-bit) to the disassembly flow. Prevent the disassembler from ingesting this memory address, as it may occasionally decode into a seemingly valid but incorrect instruction. Fix this issue by truncating the last 8/4 bytes from JITed buffers before supplying them for disassembly.
--- .../selftests/bpf/jit_disasm_helpers.c | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
tools/testing/selftests/bpf/jit_disasm_helpers.c +24 -1
@@ -179,9 +179,11 @@ int get_jited_program_text(int fd, char *text, size_t text_sz)struct bpf_prog_info info = {};__u32 info_len =sizeof (info);__u32 jited_funcs, len, pc; +__u32 trunc_len =0 , disasm_len;__u32 *func_lens =NULL ;FILE *text_out =NULL ;uint8_t *image =NULL ; +char *triple =NULL ;int i, err =0 ;if (!llvm_initialized) {
@@ -225,9 +227,30 @@ int get_jited_program_text(int fd, char *text, size_t text_sz)if (!ASSERT_OK (err,"bpf_prog_get_info_by_fd #2" )) goto out; +/* +* last 8 bytes contains dummy_trampoline address in JIT +* output on 64-bit and last 4 bytes on 32-bit powerpc, +* which can't disassemble to a valid instruction. +*/ + triple =LLVMGetDefaultTargetTriple (); +if (triple) { +if (strstr (triple,"powerpc64" ) ||strstr (triple,"ppc64" )) + trunc_len =8 ; +else if (strstr (triple,"powerpc" ) ||strstr (triple,"ppc" )) + trunc_len =4 ; +LLVMDisposeMessage (triple); + } +for (pc =0 , i =0 ; i < jited_funcs; ++i) { +fprintf (text_out,"func #%d:\n" , i); -disasm_one_func (text_out, image + pc, func_lens[i]); +/* +* Disabled JIT have zero func_lens, hence underflow +*/ + disasm_len = func_lens[i] > trunc_len ? + func_lens[i] - trunc_len :0 ; +disasm_one_func (text_out, image + pc, disasm_len); +fprintf (text_out,"\n" ); pc += func_lens[i]; }
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v11 3/8] selftests/bpf: Fixing powerpc JIT disassembly failure
This isn't a bug, but are the two new blank lines inside this loop intentional? Dropping them would keep the diff to just the truncation logic and match how the loops in disasm_one_func() are laid…
Quoted 4 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +0
-0
[ ... ]
Quoted 10 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +6
-0@@ -225,9 +227,30 @@ int get_jited_program_text(int fd, char *text, size_t text_sz)if (!ASSERT_OK (err,"bpf_prog_get_info_by_fd #2" )) goto out; +/* +* last 8 bytes contains dummy_trampoline address in JIT +* output on 64-bit and last 4 bytes on 32-bit powerpc, +* which can't disassemble to a valid instruction. +*/ + triple =LLVMGetDefaultTargetTriple ();
tools/testing/selftests/bpf/jit_disasm_helpers.c int get_jited_program_text(int fd, char *text, size_t text_sz) · line 245
Quoted 10 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +9
-0+if (triple) { +if (strstr (triple,"powerpc64" ) ||strstr (triple,"ppc64" )) + trunc_len =8 ; +else if (strstr (triple,"powerpc" ) ||strstr (triple,"ppc" )) + trunc_len =4 ; +LLVMDisposeMessage (triple); + } +for (pc =0 , i =0 ; i < jited_funcs; ++i) { +
This isn't a bug, but are the two new blank lines inside this loop intentional? Dropping them would keep the diff to just the truncation logic and match how the loops in disasm_one_func() are laid out.
Quoted 12 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +1
-0fprintf (text_out,"func #%d:\n" , i);/* * Disabled JIT have zero func_lens, hence underflow */ disasm_len = func_lens[i] > trunc_len ? func_lens[i] - trunc_len :0 ;disasm_one_func (text_out, image + pc, disasm_len); +fprintf (text_out,"\n" ); pc += func_lens[i]; }
Quoted 1 line
selftests/bpf: Fixing powerpc JIT disassembly failure
This isn't a bug, but would the subject read better in the imperative mood, "selftests/bpf: Fix powerpc JIT disassembly failure", to match the rest of the series?
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31176427801
PATCH v11 4/8 selftests/bpf: Enable verifier selftest for powerpc64
In reply to a message not in the archiveFrom: Abhishek Dubey <[email protected]>
Enable "__arch_powerpc64" specifier in verifier selftest for ppc64. Power 32-bit would require separate handling. Changes tested for 64-bit only.
--- tools/testing/selftests/bpf/progs/bpf_misc.h | 1 + tools/testing/selftests/bpf/test_loader.c | 5 +++++ 2 files changed, 6 insertions(+)
tools/testing/selftests/bpf/progs/bpf_misc.h +1 -0
@@ -159,6 +159,7 @@#define __arch_riscv64 __arch("RISCV64")#define __arch_s390x __arch("s390x")#define __arch_loongarch __arch("LOONGARCH") +#define __arch_powerpc64 __arch("POWERPC64")#define __caps_unpriv (caps) __test_tag("test_caps_unpriv=" EXPAND_QUOTE(caps))#define __load_if_JITed () __test_tag("load_mode=jited")#define __load_if_no_JITed () __test_tag("load_mode=no_jited")
tools/testing/selftests/bpf/test_loader.c +5 -0
@@ -378,6 +378,7 @@ enum arch {ARCH_RISCV64 =0x8 ,ARCH_S390X =0x10 ,ARCH_LOONGARCH =0x20 , + ARCH_POWERPC64 = 0x40, };static int get_current_arch (void )
@@ -392,6 +393,8 @@ static int get_current_arch(void)return ARCH_S390X ;#elif defined(__loongarch__)return ARCH_LOONGARCH ; +#elif defined(__powerpc64__) +return ARCH_POWERPC64 ;#endif return ARCH_UNKNOWN ; }
@@ -585,6 +588,8 @@ static int parse_test_spec(struct test_loader *tester, arch =ARCH_S390X ; }else if (strcmp (val,"LOONGARCH" ) ==0 ) { arch =ARCH_LOONGARCH ; + }else if (strcmp (val,"POWERPC64" ) ==0 ) { + arch =ARCH_POWERPC64 ; }else {PRINT_FAIL ("bad arch spec: '%s'\n" , val); err = -EINVAL ;
Signature · 1 line
2.54.0
PATCH v11 5/8 powerpc64/bpf: fix compare instruction emitted for tailcall
In reply to a message not in the archiveFrom: Abhishek Dubey <[email protected]>
The tail_call_info field can contain either a scalar counter value or a 64-bit pointer to the counter, using a 32-bit compare (cmplwi) only checks the lower 32 bits, which can lead to incorrect comparisions when location of counter is near 4GB boundary. Use instruction cmpldi/cmplwi for accurate comparision in corresponding cases.
--- arch/powerpc/net/bpf_jit.h | 6 ++++++ arch/powerpc/net/bpf_jit_comp.c | 2 +- arch/powerpc/net/bpf_jit_comp64.c | 8 ++++---- 3 files changed, 11 insertions(+), 5 deletions(-)
arch/powerpc/net/bpf_jit.h +6 -0
@@ -188,6 +188,12 @@ struct codegen_context {#define bpf_to_ppc (r) (ctx->b2p[r]) +#ifdef CONFIG_PPC64 +#define PPC_RAW_CMPLLI (a, i) PPC_RAW_CMPLDI(a, i) +#else +#define PPC_RAW_CMPLLI (a, i) PPC_RAW_CMPLWI(a, i) +#endif +#ifdef CONFIG_PPC32 #define BPF_FIXUP_LEN 3/* Three instructions => 12 bytes */ #else
arch/powerpc/net/bpf_jit_comp.c +1 -1
@@ -768,7 +768,7 @@ static void bpf_trampoline_setup_tail_call_info(u32 *image, struct codegen_conte* Setting the tail_call_info in trampoline's frame * depending on if previous frame had value or reference. */ -EMIT (PPC_RAW_CMPLWI (_R3,MAX_TAIL_CALL_CNT )); +EMIT (PPC_RAW_CMPLLI (_R3,MAX_TAIL_CALL_CNT ));PPC_BCC_CONST_SHORT (COND_GT ,8 );EMIT (PPC_RAW_ADDI (_R3, _R4, -BPF_PPC_TAILCALL ));
arch/powerpc/net/bpf_jit_comp64.c +4 -4
@@ -276,7 +276,7 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)*/ EMIT (PPC_RAW_LD (bpf_to_ppc (TMP_REG_2 ), _R1,0 ));EMIT (PPC_RAW_LD (bpf_to_ppc (TMP_REG_1 ),bpf_to_ppc (TMP_REG_2 ), -(BPF_PPC_TAILCALL ))); -EMIT (PPC_RAW_CMPLWI (bpf_to_ppc (TMP_REG_1 ),MAX_TAIL_CALL_CNT )); +EMIT (PPC_RAW_CMPLDI (bpf_to_ppc (TMP_REG_1 ),MAX_TAIL_CALL_CNT ));PPC_BCC_CONST_SHORT (COND_GT ,8 );EMIT (PPC_RAW_ADDI (bpf_to_ppc (TMP_REG_1 ),bpf_to_ppc (TMP_REG_2 ), -(BPF_PPC_TAILCALL )));
@@ -662,7 +662,7 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 oPPC_BCC_SHORT (COND_GE , out);EMIT (PPC_RAW_LD (bpf_to_ppc (TMP_REG_1 ), _R1,bpf_jit_stack_tailcallinfo_offset (ctx))); -EMIT (PPC_RAW_CMPLWI (bpf_to_ppc (TMP_REG_1 ),MAX_TAIL_CALL_CNT )); +EMIT (PPC_RAW_CMPLDI (bpf_to_ppc (TMP_REG_1 ),MAX_TAIL_CALL_CNT ));PPC_BCC_CONST_SHORT (COND_LE ,8 );/* dereference TMP_REG_1 */
@@ -672,7 +672,7 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o* if (tail_call_info == MAX_TAIL_CALL_CNT) * goto out; */ -EMIT (PPC_RAW_CMPLWI (bpf_to_ppc (TMP_REG_1 ),MAX_TAIL_CALL_CNT )); +EMIT (PPC_RAW_CMPLDI (bpf_to_ppc (TMP_REG_1 ),MAX_TAIL_CALL_CNT ));PPC_BCC_SHORT (COND_EQ , out);/*
@@ -707,7 +707,7 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o* tail_call_info. */ EMIT (PPC_RAW_LD (bpf_to_ppc (TMP_REG_2 ), _R1,bpf_jit_stack_tailcallinfo_offset (ctx))); -EMIT (PPC_RAW_CMPLWI (bpf_to_ppc (TMP_REG_2 ),MAX_TAIL_CALL_CNT )); +EMIT (PPC_RAW_CMPLDI (bpf_to_ppc (TMP_REG_2 ),MAX_TAIL_CALL_CNT ));PPC_BCC_CONST_SHORT (COND_GT ,8 );/* First get address of tail_call_info */
Signature · 1 line
2.54.0
PATCH v11 6/8 selftests/bpf: Add tailcall verifier selftest for powerpc64
In reply to a message not in the archiveFrom: Abhishek Dubey <[email protected]>
Verifier testcase result for tailcalls:
Summary: 2/3 PASSED, 0 SKIPPED, 0 FAILED
--- .../bpf/progs/verifier_tailcall_jit.c | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+)
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +75 -0
@@ -91,6 +91,81 @@ __jited(" popq %rax") __jited(" jmp {{.*}}") /* jump to tail call tgt */ __jited("L0: leave") __jited(" {{(retq|jmp 0x)}}") /* return or jump to rethunk */ +__arch_powerpc64 +/* program entry for main(), regular function prologue */ +__jited(" nop") +__jited("...") /* ld 2, 16(13) absent with CONFIG_PPC_KERNEL_PCREL */ +__jited(" li 9, 0") +__jited(" std 9, -8(1)") +__jited(" mflr 0") +__jited(" std 0, 16(1)") +__jited (" stdu 1, {{.*}}(1)" ) +/* load address and call sub() via count register +* +* Address materialization differs between PCREL and non-PCREL kernels. +* Skip the address generation sequence and verify only that the call +* target is loaded into CTR before branching. + */ +__jited("...") +__jited("...") +__jited("...") +__jited("...") +__jited("...") +__jited(" mtctr 12") +__jited(" bctrl") +__jited(" mr 8, 3") +__jited(" li 8, 0") +__jited(" addi 1, 1, {{.*}}") +__jited(" ld 0, 16(1)") +__jited(" mtlr 0") +__jited(" mr 3, 8") +__jited(" blr") +__jited("...") +__jited ("func #1" ) +/* subprogram entry for sub() */ +__jited(" nop") +__jited ("..." )/* ld 2, 16(13) absent with CONFIG_PPC_KERNEL_PCREL */ +/* tail call prologue for subprogram */ +__jited(" ld 10, 0(1)") +__jited(" ld 9, -8(10)") +__jited(" cmpldi 9, 33") +__jited(" bt {{.*}}, {{.*}}") +__jited(" addi 9, 10, -8") +__jited(" std 9, -8(1)") +__jited(" lis {{.*}}, {{.*}}") +__jited(" sldi {{.*}}, {{.*}}, 32") +__jited(" oris {{.*}}, {{.*}}, {{.*}}") +__jited(" ori {{.*}}, {{.*}}, {{.*}}") +__jited(" li {{.*}}, 0") +__jited(" lwz 9, {{.*}}({{.*}})") +__jited(" slwi {{.*}}, {{.*}}, 0") +__jited(" cmplw {{.*}}, 9") +__jited (" bf 0, {{.*}}" ) +/* bpf_tail_call implementation */ +__jited(" ld 9, -8(1)") +__jited(" cmpldi 9, 33") +__jited(" bf {{.*}}, {{.*}}") +__jited(" ld 9, 0(9)") +__jited(" cmpldi 9, 33") +__jited(" bt {{.*}}, {{.*}}") +__jited(" addi 9, 9, 1") +__jited(" mulli 10, {{.*}}, 8") +__jited(" add 10, 10, {{.*}}") +__jited(" ld 10, {{.*}}(10)") +__jited(" cmpldi 10, 0") +__jited(" bt {{.*}}, {{.*}}") +__jited(" ld 10, {{.*}}(10)") +__jited(" addi 10, 10, {{.*}}") /* offset depends on CONFIG_PPC_KERNEL_PCREL */ +__jited(" mtctr 10") +__jited(" ld 10, -8(1)") +__jited(" cmpldi 10, 33") +__jited(" bt {{.*}}, {{.*}}") +__jited(" addi 10, 1, -8") +__jited(" std 9, 0(10)") +__jited(" bctr") +__jited(" mr 3, 8") +__jited(" blr") + SEC("tc")__naked int main (void ) {
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v11 6/8] selftests/bpf: Add tailcall verifier selftest for powerpc64
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Multi-line comment in the BPF subsystem does not follow the required style. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Multi-line comment in the BPF subsystem does not follow the required style. --
--- Patch [6]: [PATCH v11 6/8] selftests/bpf: Add tailcall verifier selftest for powerpc64 --- commit db11673742dde393ab8f7429a8221e1b2ec5a904
Author: Abhishek Dubey <[email protected]>
selftests/bpf: Add tailcall verifier selftest for powerpc64
This commit adds a BPF verifier tailcall JIT testcase for powerpc64 to the selftests suite. It introduces the expected JIT instruction sequences for tail calls on the architecture.
Quoted 5 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +0
-0@@ -91,6 +91,81 @@ __jited(" popq %rax")
[ ... ]
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c __jited(" popq %rax") · line 108
Quoted 10 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +10
-0+__jited(" std 9, -8(1)") +__jited(" mflr 0") +__jited(" std 0, 16(1)") +__jited (" stdu 1, {{.*}}(1)" ) +/* load address and call sub() via count register +* +* Address materialization differs between PCREL and non-PCREL kernels. +* Skip the address generation sequence and verify only that the call +* target is loaded into CTR before branching. +*/
[Severity: Low]
Does this multi-line comment follow the modern BPF subsystem style rules?
The BPF subsystem style requires the opening /* of a multi-line comment to be on its own line, rather than having text on the same line. Should the comment start with a standalone /* to match the expected format?
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
PATCH v11 7/8 powerpc/bpf: fix buffer overflow in JIT for large BPF programs
In reply to a message not in the archiveFrom: Abhishek Dubey <[email protected]>
During size calculation in pass-0, exit_addr is 0 since addrs[fp->len] is not yet populated. bpf_jit_emit_exit_insn() treats a zero exit_addr as in-range and skips bpf_jit_build_epilogue(), so the alternate inline epilogue instructions are not counted in alloclen.
In later passes, if the real exit_addr falls outside the 32MB branch range, the full inline epilogue is emitted into the already-allocated buffer, writing past its end and corrupting adjacent memory.
Fix by ensuring exit_addr is non-zero before treating it as in-range, so pass-0 always falls through to bpf_jit_build_epilogue() and conservatively accounts for all epilogue instructions in alloclen. Also range check alt_exit_addr directly in the else-if condition.
Since exit_addr handling now falls through to the epilogue, two related issues in bpf_int_jit_compile() must also be addressed:
1. Reset cgctx.alt_exit_addr before the second size-calculation pass. Without this, a stale alt_exit_addr from the first pass causes the second pass to emit a single jump instead of the full epilogue, undercounting alloclen and reintroducing the overflow.
2. Recompute addrs[fp->len] at the end of each code-generation pass. The larger pass-0 body can shrink in later passes as out-of-range exits settle into in-range jumps; a stale addrs[fp->len] would leave exit branches targeting past the real (shrunken) epilogue.
Because shrinkage in a later pass can move the epilogue offset, the fixed two-pass loop is no longer sufficient: an exit that was out of range in an earlier pass may fall in range once the epilogue offset shrinks, shrinking the body further and overwriting the start of the epilogue. Convert the code-generation loop to iterate until the program size converges, bounded by CODEGEN_MAX_PASSES, and fail the JIT if it does not converge.
--- arch/powerpc/net/bpf_jit.h | 7 +++++++ arch/powerpc/net/bpf_jit_comp.c | 34 +++++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 8 deletions(-)
arch/powerpc/net/bpf_jit.h +7 -0
@@ -14,6 +14,13 @@#include <asm/ppc-opcode.h> #include <linux/build_bug.h> +/* +* We need at least 2 passes for proper code generation, and may need +* additional passes if code size changes between passes. +*/ +#define CODEGEN_MIN_PASSES 2 +#define CODEGEN_MAX_PASSES 3 +#ifdef CONFIG_PPC64_ELF_ABI_V1 #define FUNCTION_DESCR_SIZE 24#else
arch/powerpc/net/bpf_jit_comp.c +26 -8
@@ -128,11 +128,10 @@ void bpf_jit_build_fentry_stubs(u32 *image, u32 *fimage, struct codegen_contextint bpf_jit_emit_exit_insn (u32 *image,u32 *fimage,struct codegen_context *ctx,int tmp_reg,long exit_addr) { -if (!exit_addr ||is_offset_in_branch_range (exit_addr - (ctx->idx *4 ))) { +if (exit_addr &&is_offset_in_branch_range (exit_addr - (long )(ctx->idx *4 ))) {PPC_JMP (exit_addr); - }else if (ctx->alt_exit_addr) { -if (WARN_ON (!is_offset_in_branch_range ((long )ctx->alt_exit_addr - (ctx->idx *4 )))) -return -1 ; + }else if (ctx->alt_exit_addr &&is_offset_in_branch_range ( + (long )(ctx->alt_exit_addr) - (long )(ctx->idx *4 ))) {PPC_JMP (ctx->alt_exit_addr); }else { ctx->alt_exit_addr = ctx->idx *4 ;
@@ -303,6 +302,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr*/ if (cgctx.seen &SEEN_TAILCALL || !is_offset_in_branch_range ((long )cgctx.idx *4 )) { cgctx.idx =0 ; + cgctx.alt_exit_addr =0 ;if (bpf_jit_build_body (fp,NULL ,NULL , &cgctx, addrs,0 , false)) goto out_err; }
@@ -335,10 +335,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr code_base = (u32 *)(image +FUNCTION_DESCR_SIZE ); fcode_base = (u32 *)(fimage +FUNCTION_DESCR_SIZE ); -/* Code generation passes 1-2 */ -for (pass =1 ; pass <3 ; pass++) { +/* Code generation passes 1-2+, loop until program size converges. */ +for (pass =1 ; pass <=CODEGEN_MAX_PASSES ; pass++) { +u32 prev_proglen = proglen; +/* Now build the prologue, body code & epilogue for real. */ cgctx.idx =0 ; + cgctx.exentry_idx =0 ; cgctx.alt_exit_addr =0 ;bpf_jit_build_prologue (code_base, &cgctx); if (bpf_jit_build_body(fp, code_base, fcode_base, &cgctx, addrs, pass,
@@ -347,11 +350,26 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_prbpf_jit_binary_pack_free (fhdr, hdr); goto out_err; } + addrs[fp->len] = cgctx.idx *4 ;bpf_jit_build_epilogue (code_base, fcode_base, &cgctx); + proglen = cgctx.idx *4 ; +if (bpf_jit_enable >1 )pr_info ("Pass %d: shrink = %d, seen = 0x%x\n" , pass, - proglen - (cgctx.idx *4 ), cgctx.seen); + prev_proglen - proglen, cgctx.seen); + +/* Check if program size has converged, but ensure minimum passes */ +if (pass >=CODEGEN_MIN_PASSES && proglen == prev_proglen) +break ; + +if (pass ==CODEGEN_MAX_PASSES && proglen != prev_proglen) { +pr_err ("BPF JIT: Program did not converge after %d passes\n" , +CODEGEN_MAX_PASSES ); +bpf_arch_text_copy (&fhdr->size, &hdr->size,sizeof (hdr->size)); +bpf_jit_binary_pack_free (fhdr, hdr); + goto out_err; + } }if (bpf_jit_enable >1 )
@@ -428,7 +446,7 @@ int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, u32 *fimage, int passu32 *fixup;/* Populate extable entries only in the last pass */ -if (pass !=2 ) +if (pass <CODEGEN_MIN_PASSES )return 0 ; if (!fp->aux->extable ||
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v11 7/8] powerpc/bpf: fix buffer overflow in JIT for large BPF programs
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Silent branch offset truncation in exception table fixup generation. --
--- Patch [7]: [PATCH v11 7/8] powerpc/bpf: fix buffer overflow in JIT for large BPF programs --- commit 575dc92c1d10e63b9fc1ecf052e4cace5279974f
Author: Abhishek Dubey <[email protected]>
Subject: powerpc/bpf: fix buffer overflow in JIT for large BPF programs
This commit fixes a buffer overflow during JIT compilation of large BPF programs on PowerPC. It addresses an issue where the exit address was unpopulated in the first pass, skipping the epilogue size calculation and causing a buffer overflow in subsequent passes when the real exit address required a full inline epilogue. The patch updates the multi-pass logic to loop until the program size converges.
Quoted 4 lines
arch/powerpc/net/bpf_jit_comp.c +0
-0
[ ... ]
arch/powerpc/net/bpf_jit_comp.c int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, u32 *fimage, int pass · line 452
Quoted 9 lines
arch/powerpc/net/bpf_jit_comp.c +1
-1@@ -428,7 +446,7 @@ int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, u32 *fimage, int passu32 *fixup;/* Populate extable entries only in the last pass */ -if (pass !=2 ) +if (pass <CODEGEN_MIN_PASSES )return 0 ; if (!fp->aux->extable ||
[Severity: High]
This is a pre-existing issue, but since this patch explicitly targets support for large BPF programs (exceeding the 32MB branch range), does the exception table fixup generation also need an out-of-range branch check?
Looking at bpf_add_extable_entry() just below this hunk:
fixup[BPF_FIXUP_LEN - 1] =
PPC_RAW_BRANCH((long)(pc + jmp_off) - (long)&fixup[BPF_FIXUP_LEN - 1]);Since PPC_RAW_BRANCH silently masks out-of-range offsets without returning an error (unlike PPC_JMP which uses is_offset_in_branch_range()), could a large BPF program with exception handlers result in a truncated branch offset?
If a program has PROBE_MEM accesses and the offset between the fixup routine and the main program exceeds 32MB, it seems the CPU might jump to a corrupted address after handling the exception. Is it worth validating the branch offset here as well?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v11 8/8 powerpc64/bpf: fix percpu private stack leak on JIT failure
In reply to a message not in the archiveFrom: Abhishek Dubey <[email protected]>
The existing conditional statement in bpf_int_jit_compile() frees the percpu private stack at out_addrs only when the image buffer was never allocated.
If bpf_jit_build_body() fails during a code-generation pass, the image buffer has already been allocated, so !image is false and the percpu stack is not freed.
Because JIT compilation failed, fp->jited remains at 0. The subsequent bpf_jit_free() path only frees priv_stack_ptr when fp->jited is set, so freeing is skipped here too, leaking the percpu allocation.
Fix implements freeing the private stack whenever fp->jited was not set, i.e. compilation did not succeed, instead of keying off !image. !fp->jited already covers the !image case, since image is only NULL on early-failure paths where fp->jited is likewise 0.
--- arch/powerpc/net/bpf_jit_comp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/net/bpf_jit_comp.c +1 -1
@@ -404,7 +404,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr (void *)fimage + FUNCTION_DESCR_SIZE); out_addrs: -if (!image && priv_stack_ptr) { +if (!fp->jited && priv_stack_ptr) { fp->aux->priv_stack_ptr =NULL ;free_percpu (priv_stack_ptr); }
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v11 8/8] powerpc64/bpf: fix percpu private stack leak on JIT failure
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Critical] In bpf_jit_binary_pack_finalize(), if bpf_arch_text_copy() fails, it uses uninitialized ro_header->size to free the pack (causing bitmap corruption) and fails to uncharge modmem.
- [Critical] If bpf_jit_binary_pack_finalize() fails during bpf_jit_free(), the JIT memory pack is double-freed.
- [High] The ARM64 BPF JIT compiler has the exact same percpu private stack memory leak on failure paths that this patch fixes for PowerPC.
--
--- Patch [8]: [PATCH v11 8/8] powerpc64/bpf: fix percpu private stack leak on JIT failure --- commit e1be4be6e638db0714dd1f4ef6a5f98e6f888db6
Author: Abhishek Dubey <[email protected]>
powerpc64/bpf: fix percpu private stack leak on JIT failure
The existing conditional statement in bpf_int_jit_compile() frees the percpu private stack at out_addrs only when the image buffer was never allocated.
arch/powerpc/net/bpf_jit_comp.c struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr · line 410
Quoted 13 lines
arch/powerpc/net/bpf_jit_comp.c +1
-1@@ -404,7 +404,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr (void *)fimage + FUNCTION_DESCR_SIZE); out_addrs: -if (!image && priv_stack_ptr) { +if (!fp->jited && priv_stack_ptr) { fp->aux->priv_stack_ptr =NULL ;free_percpu (priv_stack_ptr); }
[Severity: High]
This isn't a bug introduced by this patch, but the ARM64 BPF JIT compiler has the exact same percpu private stack memory leak on failure paths that this patch addresses for PowerPC.
In arch/arm64/net/bpf_jit_comp.c:bpf_int_jit_compile():
out_off:
if (!ro_header && priv_stack_ptr) {
free_percpu(priv_stack_ptr);
prog->aux->priv_stack_ptr = NULL;
}On failure paths jumping from out_free_hdr, ro_header is not NULL, causing the percpu stack free to be skipped. Could this also exhaust percpu memory on ARM64?
[Severity: Critical]
This is a pre-existing issue, but if bpf_jit_binary_pack_finalize() fails during bpf_jit_free(), does this double-free the JIT memory pack?
In arch/powerpc/net/bpf_jit_comp.c:bpf_jit_free():
if (jit_data) {
bpf_jit_binary_pack_finalize(jit_data->fhdr,
jit_data->hdr);
kvfree(jit_data->addrs);
kfree(jit_data);
}
hdr = bpf_jit_binary_pack_hdr(fp);
bpf_jit_binary_pack_free(hdr, NULL);If jit_subprogs() aborts early and calls bpf_jit_free() on subprograms, bpf_jit_binary_pack_finalize() is called. If its internal text copy fails, it internally frees ro_header. However, the return value of bpf_jit_binary_pack_finalize() is ignored here, and bpf_jit_binary_pack_free() is called again, which seems to double-free the same pack structure.
The same pattern appears to exist in arch/x86/net/bpf_jit_comp.c:bpf_jit_free() as well.
[Severity: Critical]
This is a pre-existing issue, but does bpf_jit_binary_pack_finalize() use uninitialized memory if bpf_arch_text_copy() fails?
In kernel/bpf/core.c:bpf_jit_binary_pack_finalize():
if (IS_ERR(ptr)) {
bpf_prog_pack_free(ro_header, ro_header->size);
return PTR_ERR(ptr);
}If bpf_arch_text_copy() fails, ro_header->size has not been initialized. Could this clear arbitrary memory regions in the shared pack allocator bitmap and permanently leak the module memory charge since bpf_jit_uncharge_modmem() is missing on this error path?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8