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 v10 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 | 4 ++-- arch/powerpc/net/bpf_jit_comp.c | 39 +++++++++++++++++++++++++++---- arch/powerpc/net/bpf_jit_comp32.c | 4 ++-- arch/powerpc/net/bpf_jit_comp64.c | 4 ++-- 4 files changed, 40 insertions(+), 11 deletions(-)
arch/powerpc/net/bpf_jit.h +2 -2
@@ -214,8 +214,8 @@ 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); void prepare_for_fsession_fentry(u32 *image, struct codegen_context *ctx, int cookie_cnt,
arch/powerpc/net/bpf_jit_comp.c +34 -5
@@ -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));
@@ -107,7 +136,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,NULL , ctx); }return 0 ;
@@ -286,7 +315,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 +347,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_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); }/*
Signature · 1 line
2.54.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Passing `NULL` for `fimage` in `bpf_jit_emit_exit_insn()` causes incorrect alignment calculations…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Passing NULL for fimage in bpf_jit_emit_exit_insn() causes incorrect alignment calculations for alternate epilogues, leading to an unaligned 8-byte memory store during JIT compilation and misaligned runtime addresses. --
--- Patch [1]: [PATCH v10 1/8] powerpc/bpf: fix alignment of long branch trampoline address --- commit 4bc0d2265cceaf063f46ec43f44292e01148338a
Author: Abhishek Dubey <[email protected]>
powerpc/bpf: fix alignment of long branch trampoline address
This commit attempts to fix the alignment of the dummy trampoline address field between the out-of-line stub and the long branch stub. It introduces a NOP padding instruction to ensure the address is 8-byte aligned for load-register compatibility. It also passes fimage through the epilogue and fentry stub building functions to calculate the correct alignment.
Quoted 11 lines
arch/powerpc/net/bpf_jit_comp.c +1
-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) {
[ ... ]
Quoted 4 lines
arch/powerpc/net/bpf_jit_comp.c +4
-0+ 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 ()); +
[ ... ]
Quoted 8 lines
arch/powerpc/net/bpf_jit_comp.c +1
-0@@ -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));
[ ... ]
arch/powerpc/net/bpf_jit_comp.c int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg, · line 142
Quoted 9 lines
arch/powerpc/net/bpf_jit_comp.c +1
-1@@ -107,7 +136,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,NULL , ctx); }return 0 ;
[Severity: High]
Does passing NULL for fimage here cause incorrect alignment calculations for alternate epilogues?
Since bpf_jit_emit_exit_insn() passes NULL to bpf_jit_build_epilogue(), the fimage argument eventually passed to bpf_jit_build_fentry_stubs() will be NULL. This means the IS_ALIGNED() check in bpf_jit_build_fentry_stubs() will evaluate alignment relative to 0 instead of the actual fcode_base.
Because the executable memory allocated by bpf_jit_binary_pack_alloc() only guarantees 4-byte alignment, the real fimage base might not be 8-byte aligned. If the NOP calculation evaluates 0 instead of a 4-byte aligned base, it can incorrectly omit the padding, leading to an unaligned 64-bit store when writing dummy_tramp:
if (image) *((unsigned long *)&image[ctx->idx]) = (unsigned long)dummy_tramp;
Performing a 64-bit store to a misaligned pointer is undefined behavior in C and can trigger alignment exceptions. Will this also cause the ld instruction to load from a misaligned address at runtime, defeating the patch's goal of load-register compatibility?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Hari Bathini
maintainerQuoted 5 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.
This fix is not complete. With 2/8 it looks complete. Some part of it needs to be in this patch instead..
Quoted 154 lines
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 | 4 ++-- arch/powerpc/net/bpf_jit_comp.c | 39 +++++++++++++++++++++++++++---- arch/powerpc/net/bpf_jit_comp32.c | 4 ++-- arch/powerpc/net/bpf_jit_comp64.c | 4 ++-- 4 files changed, 40 insertions(+), 11 deletions(-)arch/powerpc/net/bpf_jit.h +2
-2@@ -214,8 +214,8 @@ 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); void prepare_for_fsession_fentry(u32 *image, struct codegen_context *ctx, int cookie_cnt,arch/powerpc/net/bpf_jit_comp.c +34
-5@@ -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));@@ -107,7 +136,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,NULL , ctx); }return 0 ;@@ -286,7 +315,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 +347,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_comp64.c +1
-1@@ -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);
arch/powerpc/net/bpf_jit_comp64.c void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx) · line 413
Quoted 8 lines
arch/powerpc/net/bpf_jit_comp64.c +1
-1@@ -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); }/*
- Hari
PATCH v10 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 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.h | 3 +- arch/powerpc/net/bpf_jit_comp.c | 51 ++++++++++++++++--------------- arch/powerpc/net/bpf_jit_comp32.c | 3 +- arch/powerpc/net/bpf_jit_comp64.c | 3 +- 4 files changed, 33 insertions(+), 27 deletions(-)
arch/powerpc/net/bpf_jit.h +2 -1
@@ -217,7 +217,8 @@ void bpf_jit_build_prologue(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 +27 -24
129 lines not on this page · raw message
arch/powerpc/net/bpf_jit_comp32.c +2 -1
@@ -1149,7 +1149,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, _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 +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; }
Signature · 1 line
2.54.0
Hari Bathini
maintainerRe: [PATCH v10 2/8] powerpc/bpf: Move out dummy_tramp_addr after Long branch stub
Quoted 6 lines · Saket Kumar Bhaskar
From: 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 8 bytes. The last bytes exclusion is logically necessary to
Instead of "last 8 bytes", should be "last 4/8 bytes"
Quoted 45 lines
prevent disassembly failure, otherwise the actual program layout is never altered. Hence no effect on overall program size. Also, align
dummy_tramp_addrfield with 8-byte boundary.Following is disassembler output for test program with moved down
dummy_tramp_addrfield:
.....
.....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_addrfield offset calculation from the end of the program to reflect its new location, forbpf_arch_text_poke()to update the actual trampoline's address in this field.All BPF trampoline selftests continue to pass with this patch applied.
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 | 3 +- arch/powerpc/net/bpf_jit_comp.c | 51 ++++++++++++++++--------------- arch/powerpc/net/bpf_jit_comp32.c | 3 +- arch/powerpc/net/bpf_jit_comp64.c | 3 +- 4 files changed, 33 insertions(+), 27 deletions(-)arch/powerpc/net/bpf_jit.h +0
-0
arch/powerpc/net/bpf_jit.h void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx); · line 222
Quoted 7 lines
arch/powerpc/net/bpf_jit.h +2
-1@@ -217,7 +217,8 @@ void bpf_jit_build_prologue(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);
This function prototype change, the corresponding call site updates & the second parameter update in bpf_jit_build_epilogue() call inside bpf_jit_emit_exit_insn() function belong in the previous patch...
Quoted 55 lines
arch/powerpc/net/bpf_jit.h +0
-0void 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 +8
-11@@ -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,35 +96,37 @@ 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
arch/powerpc/net/bpf_jit_comp.c void bpf_jit_build_fentry_stubs(u32 *image, u32 *fimage, struct codegen_context · line 107
Quoted 10 lines
arch/powerpc/net/bpf_jit_comp.c +4
-3-* 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> // 8-byte aligned
Likewise, the comment should be:
"SZL bytes aligned"
Quoted 99 lines
arch/powerpc/net/bpf_jit_comp.c +8
-6*/ -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 ; } } -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);arch/powerpc/net/bpf_jit_comp.c +7
-4@@ -136,7 +136,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,NULL , ctx); +bpf_jit_build_epilogue (image, fimage, ctx); }return 0 ;@@ -1294,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> @@ -1301,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. * @@ -1410,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)arch/powerpc/net/bpf_jit_comp32.c +2
-1@@ -1149,7 +1149,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, _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 +0
-0
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; }
- Hari
PATCH v10 3/8 selftest/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 v10 3/8] selftest/bpf: Fixing powerpc JIT disassembly failure
The Fixes: tag names b991fc520700 ("selftests/bpf: utility function to get program disassembly after jit", 2024-08-21), which is the commit that introduced get_jited_program_text() itself. That…
Quoted 11 lines
selftest/bpf: Fixing powerpc JIT disassembly failure
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.
Quoted 1 line
Fixes b991fc520700 ("selftests/bpf: utility function to get program disassembly after jit")
The Fixes: tag names b991fc520700 ("selftests/bpf: utility function to get program disassembly after jit", 2024-08-21), which is the commit that introduced get_jited_program_text() itself. That helper was not wrong when
written: at that time no powerpc JIT emitted a raw data word inside a program, and the helper is documented (jit_disasm_helpers.c:13) as serving small assembly test programs.
The undisassemblable trailing bytes come from d243b62b7bd3 ("powerpc64/bpf:
Add support for bpf trampolines", 2024-10-31) - the same commit both sibling patches in this series cite - and only become observable once powerpc jited selftests are enabled, which happens later in this series in d3841599dc7ca ("selftest/bpf: Enable verifier selftest for powerpc64").
More importantly, the fix as written is only correct on top of its immediate parent. Truncating the last trunc_len bytes assumes dummy_tramp_addr is the final thing in the JITed image. That layout is established by the immediate parent c6ba4edfe6e46 ("powerpc/bpf: Move out dummy_tramp_addr after Long branch stub"), which moved the data word from the top of the long branch stub to its bottom:
arch/powerpc/net/bpf_jit_comp.c:bpf_jit_build_fentry_stubs() {
...
long_branch_stub_idx = ctx->idx;
EMIT(PPC_RAW_MFLR(_R11));
...
EMIT(PPC_RAW_BCTR());
if (image)
*((unsigned long *)&image[ctx->idx]) = (unsigned long)dummy_tramp;
ctx->idx += SZL / 4;
}On any tree that has b991fc520700 but not c6ba4edfe6e46, dummy_tramp_addr sits before the seven long-branch instructions, so trimming the tail removes two real instructions (bctr and mtlr r11 on 64-bit) while leaving the data word in the middle of the disassembled range - the disassembly failure this patch targets is not fixed, and expected __jited() output is silently altered.
A backport driven by the stated Fixes: tag alone would therefore land a change that does not work. Should the tag point at the commit that introduced the byte pattern (d243b62b7bd3), or should the dependency on c6ba4edfe6e46 be stated explicitly in the changelog?
Quoted 4 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +0
-0
[ ... ]
tools/testing/selftests/bpf/jit_disasm_helpers.c int get_jited_program_text(int fd, char *text, size_t text_sz) · line 234
Quoted 9 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +5
-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. +*/
This isn't a bug, but the comment says the trailing bytes "can't disassemble to a valid instruction", while the changelog for the same change says the opposite - "it may occasionally decode into a seemingly valid but incorrect instruction" - which is the case that actually motivates unconditional truncation rather than tolerating a disasm failure.
The comment also names the field "dummy_trampoline", whereas the arch code calls the symbol "dummy_tramp" and the field "dummy_tramp_addr" (arch/powerpc/net/bpf_jit_comp.c:118), so grepping from the comment does not lead to the code.
Separately, the second added comment, "Disabled JIT have zero func_lens, hence underflow", is a sentence fragment that leaves the reader to infer that the ternary exists to avoid a __u32 wrap when func_lens[i] is 0.
Could this comment use the "dummy_tramp_addr" name from the arch code and describe the failure the same way the changelog does, so the two do not read as contradictory?
Quoted 12 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +10
-0+ 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);
[ ... ]
tools/testing/selftests/bpf/jit_disasm_helpers.c int get_jited_program_text(int fd, char *text, size_t text_sz) · line 256
Quoted 4 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +1
-0+fprintf (text_out,"\n" ); pc += func_lens[i]; }
This isn't a bug, but two blank lines are added inside the loop body that are unrelated to the fix: one immediately after the "for (...) {" opening brace, and one between the disasm_one_func() call and the following fprintf(). Neither separates logical blocks - the loop body is six lines - and no other loop in this file opens with a blank line (compare the two "while (pc < len)" loops in disasm_one_func(), which start directly with their first statement).
Are the two new blank lines inside this loop intentional, or could the hunk stay limited to the truncation change?
Quoted 1 line
selftest/bpf: Fixing powerpc JIT disassembly failure
This isn't a bug, but the subject uses the prefix "selftest/bpf:" (singular) where the tree's overwhelmingly dominant prefix for this directory is "selftests/bpf:" - 1272 commits versus 10 in recent history. The subject also uses the gerund "Fixing" rather than the imperative mood the submitting-patches guidance asks for ("Fix ..." / "Skip ...").
Would "selftests/bpf: Skip trailing trampoline address bytes when disassembling" fit the usual prefix and imperative style for this directory better?
---
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/30981212698
Hari Bathini
maintainerRe: [PATCH v10 3/8] selftest/bpf: Fixing powerpc JIT disassembly failure
In reply to Saket Kumar BhaskarQuoted 12 lines · Saket Kumar Bhaskar
From: 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.
Quoted 1 line
Fixes b991fc520700 ("selftests/bpf: utility function to get program disassembly after jit")
The fixes tag on this patch doesn't really help. Please drop it.
Quoted 45 lines
Signed-off-by Abhishek Dubey <[email protected]>Signed-off-by Saket Kumar Bhaskar <[email protected]>--- .../selftests/bpf/jit_disasm_helpers.c | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)tools/testing/selftests/bpf/jit_disasm_helpers.c +17
-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]);
tools/testing/selftests/bpf/jit_disasm_helpers.c int get_jited_program_text(int fd, char *text, size_t text_sz) · line 256
Quoted 10 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +7
-0+/* +* 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]; }
- Hari
Re: [PATCH v10 3/8] selftest/bpf: Fixing powerpc JIT disassembly failure
Quoted 15 lines · Hari Bathini
Quoted 12 lines · Saket Kumar Bhaskar
From: 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.
Quoted 3 lines
Quoted 1 line
Fixes b991fc520700 ("selftests/bpf: utility function to get program disassembly after jit")The fixes tag on this patch doesn't really help. Please drop it.
With that..
Quoted 58 lines
Quoted 46 lines
Signed-off-by Abhishek Dubey <[email protected]>Signed-off-by Saket Kumar Bhaskar <[email protected]>---
.../selftests/bpf/jit_disasm_helpers.c | 25 ++++++++++++++++++-1 file changed, 24 insertions(+), 1 deletion(-)tools/testing/selftests/bpf/jit_disasm_helpers.c +17
-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]);tools/testing/selftests/bpf/jit_disasm_helpers.c int get_jited_program_text(int fd, char *text, · line 256
Quoted 10 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +7
-0+ /* + * 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]; }- Hari
PATCH v10 4/8 selftest/bpf: Enable verifier selftest for powerpc64
In reply to a message not in the archiveFrom: Abhishek Dubey <[email protected]>
This patch enables arch specifier "__powerpc64" 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
[email protected]
AI reviewRe: [PATCH v10 4/8] selftest/bpf: Enable verifier selftest for powerpc64
This isn't a bug, but would it read more precisely to name the new tag as __arch_powerpc64 (matching the macro added in bpf_misc.h) rather than "__powerpc64"? The actual macro expands to…
Quoted 5 lines
selftest/bpf: Enable verifier selftest for powerpc64
This patch enables arch specifier "
__powerpc64" in verifier selftest for ppc64. Power 32-bit would require separate handling. Changes tested for 64-bit only.
This isn't a bug, but would it read more precisely to name the new tag as __arch_powerpc64 (matching the macro added in bpf_misc.h) rather than "__powerpc64"? The actual macro expands to __arch("POWERPC64"), and "__powerpc64__" is the compiler predefine used in test_loader.c, but there's no __powerpc64 tag itself.
Also, would phrasing it imperatively match the series style better? For example, "Add __arch_powerpc64 so verifier tests can gate on ppc64" instead of "This patch enables..."?
Quoted 34 lines
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 +3
-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 ; }
tools/testing/selftests/bpf/test_loader.c static int parse_test_spec(struct test_loader *tester, · line 595
Quoted 8 lines
tools/testing/selftests/bpf/test_loader.c +2
-0@@ -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 ;
---
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/30981212698
Hari Bathini
maintainerRe: [PATCH v10 4/8] selftest/bpf: Enable verifier selftest for powerpc64
In reply to Saket Kumar BhaskarQuoted 4 lines · Saket Kumar Bhaskar
From: Abhishek Dubey <[email protected]>
This patch enables arch specifier "
__powerpc64" in verifier
s/This patch enables arch specifier "__powerpc64" in/Enable "__arch_powerpc64" specifier in/
Quoted 54 lines
selftest for ppc64. Power 32-bit would require separate handling. Changes tested for 64-bit only.
Signed-off-by Abhishek Dubey <[email protected]>Signed-off-by Saket Kumar Bhaskar <[email protected]>Reviewed-by Hari Bathini <[email protected]>--- 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 ;
PATCH v10 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
Re: [PATCH v10 5/8] powerpc64/bpf: fix compare instruction emitted for tailcall
Quoted 9 lines · Saket Kumar Bhaskar
From: Abhishek Dubey <[email protected]>
The
tail_call_infofield 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.
Quoted 3 lines
Reported-by [email protected]Fixes 2ed2d8f6fb38 ("powerpc64/bpf: Support tailcalls with subprogs")
Quoted 78 lines
Signed-off-by Saket Kumar Bhaskar <[email protected]>--- 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 */
PATCH v10 6/8 selftest/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 | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+)
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +74 -0
@@ -91,6 +91,80 @@ __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 v10 6/8] selftest/bpf: Add tailcall verifier selftest for powerpc64
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] The patch introduces a multi-line comment that violates the BPF subsystem comment style. - [Low]…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The patch introduces a multi-line comment that violates the BPF subsystem comment style.
- [Low] Redundant and misleading repetition of __jited("...") annotations.
--
--- Patch [6]: [PATCH v10 6/8] selftest/bpf: Add tailcall verifier selftest for powerpc64 --- commit 93fe35d1a465f1c1dd05d30ed36bf46603bc5f71
Author: Abhishek Dubey <[email protected]>
selftest/bpf: Add tailcall verifier selftest for powerpc64
This commit adds a new BPF tailcall verifier JIT selftest for the powerpc64 architecture. It verifies the emitted JIT instructions for tail calls, handling differences between PCREL and non-PCREL kernels.
Quoted 4 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +0
-0
[ ... ]
Quoted 8 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +4
-0@@ -91,6 +91,80 @@ __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 */
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c __jited(" popq %rax") · line 107
Quoted 10 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +10
-0+__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. +*/
[Severity: Low]
This isn't a bug, but does this comment block follow the standard BPF subsystem comment style?
The subsystem guidelines require multi-line comments to have the opening characters on a dedicated line, like this:
/*
* Address materialization differs between PCREL and non-PCREL kernels.
* Skip the address generation sequence...
*/
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c __jited(" popq %rax") · line 112
Quoted 5 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +5
-0+__jited("...") +__jited("...") +__jited("...") +__jited("...") +__jited ("..." )
[Severity: Low]
Are these consecutive string annotations redundant?
When test_loader.c parses the "..." string via push_disasm_msg(), it merely sets the on_next_line flag to false. Repeating it multiple times does not force the verifier to skip exactly five instructions; it just sets the same boolean flag repeatedly.
Would a single annotation be sufficient to achieve the desired skip?
Quoted 3 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +3
-0+__jited(" mtctr 12") +__jited(" bctrl") +__jited (" mr 8, 3" )
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
PATCH v10 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 v10 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: - [High] Inverted pass condition in `bpf_add_extable_entry()` causes JIT compilation failure and stale…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Inverted pass condition in bpf_add_extable_entry() causes JIT compilation failure and stale exception tables. --
--- Patch [7]: [PATCH v10 7/8] powerpc/bpf: fix buffer overflow in JIT for large BPF programs --- commit 7c6fc2cfb9f061eb19bae10ea234d06c7a76903c
Author: Abhishek Dubey <[email protected]>
powerpc/bpf: fix buffer overflow in JIT for large BPF programs
This commit modifies the code-generation loop to iterate until the program size converges, bounded by CODEGEN_MAX_PASSES, to prevent buffer overflows during JIT compilation of large BPF programs. It also ensures exit_addr is non-zero before treating it as in-range and resets alt_exit_addr before subsequent passes.
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]
Does this accidentally invert the logic for populating the exception table?
The comment says to populate extable entries only in the last pass, but returning 0 when pass >= CODEGEN_MIN_PASSES means it will skip the final code generation passes and instead execute during the early passes.
During the scouting pass 0 in bpf_int_jit_compile(), pass >= CODEGEN_MIN_PASSES evaluates to false, causing it to fall through. Since the exception table is not yet allocated at this stage, bpf_add_extable_entry() returns -EINVAL and unconditionally fails JIT compilation for any BPF program with exceptions.
Additionally, for programs taking the extra_pass path, this would cause the extable population to be skipped during the final passes. Because instructions can shrink between passes, this might leave stale extable offsets that point to incorrect locations, which could trigger a kernel panic during unhandled page faults.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
[email protected]
AI reviewRe: [PATCH v10 7/8] powerpc/bpf: fix buffer overflow in JIT for large BPF programs
In reply to Saket Kumar BhaskarWith CODEGEN_MAX_PASSES set to 3, the convergence loop only allows a single retry after the mandatory two passes. Is this sufficient margin for the cascading shrinkage described in the commit message?
Quoted 76 lines
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 +15
-7@@ -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 */
arch/powerpc/net/bpf_jit_comp.c struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr · line 372
Quoted 10 lines
arch/powerpc/net/bpf_jit_comp.c +10
-0+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; + }
With CODEGEN_MAX_PASSES set to 3, the convergence loop only allows a single retry after the mandatory two passes. Is this sufficient margin for the cascading shrinkage described in the commit message?
The changelog states that when the epilogue offset moves, an exit that was out of range can fall in range, shrinking the body and potentially triggering a chain reaction. That pattern can need more than one settling iteration for a large program. Other architectures allow significantly more headroom for this exact scenario: riscv and parisc use NR_JIT_ITERATIONS (default 16), sparc64 uses 40 passes, x86 uses MAX_PASSES (20), and mips uses JIT_MAX_ITERATIONS (32).
The tight bound has two concrete effects:
1. A program that would eventually converge can now fail the JIT hard. The old two-pass code did not check convergence at all, so a program that needs four settling iterations to fully cascade would have been JITed with pass 2 output. Now it gets -ENOTSUPP. On a CONFIG_BPF_JIT_ALWAYS_ON or bpf_jit_enable=2 kernel, that is a verifier rejection at load time, not a silent fallback to interpreter.
2. Each pass walks the entire program without preemption. x86 and sparc64 both call cond_resched() in their codegen loops. A large program will burn a full pass with preemption unchanged, and with only three passes allowed there is no headroom to add resched points.
For comparison, x86's codegen loop:
kernel/bpf/core.c:bpf_int_jit_compile() {
for (pass = 0; pass < MAX_PASSES || image; pass++) {
...
cond_resched();
}
}Would raising CODEGEN_MAX_PASSES to a value with real headroom (perhaps matching NR_JIT_ITERATIONS at 16, or 20 to match x86) and adding cond_resched() address these concerns?
arch/powerpc/net/bpf_jit_comp.c int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, u32 *fimage, int pass · line 450
Quoted 10 lines
arch/powerpc/net/bpf_jit_comp.c +1
-1}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 ;
Is this comparison inverted? The old predicate was if (pass != 2) return 0; with the fixed two-pass loop for (pass = 1; pass < 3; pass++), which populated entries only when pass == 2, the final pass.
The new code is if (pass >= CODEGEN_MIN_PASSES) return 0; with CODEGEN_MIN_PASSES == 2, which returns early for pass 2 and pass 3 and populates entries only when pass == 1. That is the exact inverse, and contradicts the retained comment "Populate extable entries only in the last pass."
The correct predicate for "not the last pass" in the new convergence loop would be if (pass < CODEGEN_MIN_PASSES) return 0;, or a last-pass flag, not pass >= CODEGEN_MIN_PASSES.
Two concrete consequences from populating during pass 1 instead of the final pass:
1. Stale extable contents. Entries are written during pass 1 using pass-1 offsets. The whole point of this patch is that the body can shrink between passes (an out-of-range exit settling into an in-range jump), so pass 2/3 instruction addresses differ from pass 1.
bpf_add_extable_entry() records deltas derived from absolute addresses:
kernel/bpf/verifier.c:bpf_add_extable_entry() {
pc = (unsigned long)&image[insn_idx];
offset = pc - (long)&ex_entry->insn;
ex_entry->insn = offset;
fixup[BPF_FIXUP_LEN - 1] = PPC_RAW_BRANCH(
(long)(pc + jmp_off) - (long)&fixup[BPF_FIXUP_LEN - 1]);
}Because cgctx.exentry_idx is now reset at the top of every pass (CHANGE-3), and pass 2+ returns early, nothing rewrites these entries after the layout changes.
search_bpf_extables() in kernel/bpf/core.c then resolves a faulting PROBE_MEM access to the wrong insn address, and the recorded fixup branch jumps to a stale offset. For a program whose size changes between pass 1 and the final pass, that is a wild branch from the fault fixup path in kernel context.
2. Interaction with extra_pass for subprogs. On the skip_init_ctx path, the comment at arch/powerpc/net/bpf_jit_comp.c:259 states:
/* During extra pass, ensure index is reset before repopulating
extable entries */
The design intent is that the extra pass repopulates the extable. With the new predicate, the extra pass enters the loop at pass == 1 and does populate. But any later iteration of the same loop (pass 2, which is always executed because CODEGEN_MIN_PASSES == 2 gates the early break) will not. That leaves the extable describing whichever pass wrote last (pass 1) while the final emitted code is from pass 2. The comment and the code now disagree in every configuration.
Would inverting the test to if (pass < CODEGEN_MIN_PASSES) return 0; fix this?
---
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/30981212698
Hari Bathini
maintainerRe: [PATCH v10 7/8] powerpc/bpf: fix buffer overflow in JIT for large BPF programs
In reply to Saket Kumar BhaskarQuoted 141 lines · Saket Kumar Bhaskar
From: Abhishek Dubey <[email protected]>
During size calculation in pass-0,
exit_addris 0 since addrs[fp->len] is not yet populated.bpf_jit_emit_exit_insn()treats a zeroexit_addras in-range and skipsbpf_jit_build_epilogue(), so the alternate inline epilogue instructions are not counted in alloclen.In later passes, if the real
exit_addrfalls 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_addris non-zero before treating it as in-range, so pass-0 always falls through tobpf_jit_build_epilogue()and conservatively accounts for all epilogue instructions in alloclen. Also range checkalt_exit_addrdirectly in the else-if condition.Since
exit_addrhandling now falls through to the epilogue, two related issues inbpf_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_addrfrom 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.Reported-by [email protected]Closes https://lore.kernel.org/bpf/[email protected]/T/#mfcb23909d977b949727cca4f59ee56a13fd69b92 open hereFixes d243b62b7bd3 ("powerpc64/bpf: Add support for bpf trampolines")Signed-off-by Hari Bathini <[email protected]>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 | 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 +25
-7@@ -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 )
arch/powerpc/net/bpf_jit_comp.c int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, u32 *fimage, int pass · line 450
Quoted 6 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 )
This is wrong. Has to be:
if (pass < CODEGEN_MIN_PASSES)
Quoted 3 lines
arch/powerpc/net/bpf_jit_comp.c +0
-0return 0 ;if (!fp->aux->extable ||
- Hari
PATCH v10 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 v10 8/8] powerpc64/bpf: fix percpu private stack leak on JIT failure
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] Percpu private stack memory is permanently leaked in ARM64 JIT on compilation error paths. --
--- Patch [8]: [PATCH v10 8/8] powerpc64/bpf: fix percpu private stack leak on JIT failure --- commit 04e1b164f2198ac765746ea52f6d89ebc5ed346a
Author: Abhishek Dubey <[email protected]>
powerpc64/bpf: fix percpu private stack leak on JIT failure
This commit fixes a percpu memory leak in the powerpc64 BPF JIT compiler. It ensures the private stack is properly freed on error paths by checking if compilation succeeded, rather than relying on whether the image buffer was 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 is a pre-existing issue, but does a similar memory leak exist in the ARM64 BPF JIT error path?
Looking at bpf_int_jit_compile() in arch/arm64/net/bpf_jit_comp.c:
out_free_hdr:
...
if (header) {
bpf_arch_text_copy(&ro_header->size, &header->size,
sizeof(header->size));
bpf_jit_binary_pack_free(ro_header, header);
}
goto out_off;out_off:
if (!ro_header && priv_stack_ptr) {
free_percpu(priv_stack_ptr);
prog->aux->priv_stack_ptr = NULL;
}If the code jumps to out_free_hdr, ro_header is freed but not set to NULL. When execution proceeds to out_off, the !ro_header check will evaluate to false.
Since prog->jited is also set to 0 on these error paths, bpf_jit_free() will skip freeing the private stack as well.
Could this allow a user to deliberately trigger JIT failures (for example, by hitting limits during passes) to repeatedly leak percpu memory and exhaust the allocator?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
Re: [PATCH v10 8/8] powerpc64/bpf: fix percpu private stack leak on JIT failure
In reply to Saket Kumar BhaskarQuoted 19 lines · Saket Kumar Bhaskar
From: Abhishek Dubey <[email protected]>
The existing conditional statement in
bpf_int_jit_compile()frees the percpu private stack atout_addrsonly 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->jitedremains at 0. The subsequentbpf_jit_free()path only freespriv_stack_ptrwhenfp->jitedis set, so freeing is skipped here too, leaking the percpu allocation.Fix implements freeing the private stack whenever
fp->jitedwas 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 wherefp->jitedis likewise 0.
Quoted 4 lines
Reported-by [email protected]Fixes 156d985123b6 ("powerpc64/bpf: Implement JIT support for private stack")
Quoted 20 lines
Signed-off-by Abhishek Dubey <[email protected]>Signed-off-by Saket Kumar Bhaskar <[email protected]>--- 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); }