powerpc/bpf: fix buffer overflow in JIT for large BPF programs
Subsystems (6)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v12 1/8 powerpc/bpf: fix buffer overflow in JIT for large BPF programs
From: 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
@@ -99,11 +99,10 @@ 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) { -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 ;
@@ -274,6 +273,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; }
@@ -306,10 +306,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,
@@ -318,11 +321,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, &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 )
@@ -399,7 +417,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.55.0
[email protected]
AI reviewThank 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] bpf_jit_binary_pack_finalize leaks modmem charge and uses uninitialized size on error path. --
--- Patch [1]: [PATCH v12 1/8] powerpc/bpf: fix buffer overflow in JIT for large BPF programs --- commit 86d46c787bf7b94485f88f3d4dc3220927360140
Author: Abhishek Dubey <[email protected]>
powerpc/bpf: fix buffer overflow in JIT for large BPF programs
This patch fixes a buffer overflow in the PowerPC BPF JIT compiler caused by incorrect handling of exit_addr during pass-0 size calculation. It ensures the epilogue instructions are conservatively accounted for in the initial allocation and iterates the code-generation loop until the program size converges.
Quoted 4 lines
arch/powerpc/net/bpf_jit_comp.c +0
-0
[ ... ]
Quoted 31 lines
arch/powerpc/net/bpf_jit_comp.c +11
-3@@ -306,10 +306,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,@@ -318,11 +321,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, &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 343
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; + }
[Severity: High]
This isn't a bug introduced by this patch, but since we are touching the JIT error paths here, I noticed a potential leak in bpf_jit_binary_pack_finalize().
Does the error path in kernel/bpf/core.c:bpf_jit_binary_pack_finalize() leak the global JIT memory charge?
When bpf_arch_text_copy() fails, the error path cleans up the memory allocations but appears to completely omit the corresponding bpf_jit_uncharge_modmem() call:
kernel/bpf/core.c:bpf_jit_binary_pack_finalize() {
...
kvfree(rw_header);
if (IS_ERR(ptr)) {
bpf_prog_pack_free(ro_header, ro_header->size);
return PTR_ERR(ptr);
}
...
}Additionally, could ro_header->size be uninitialized if bpf_arch_text_copy() fails early, potentially causing bpf_prog_pack_free() to corrupt the pack bitmap?
Quoted 1 line
arch/powerpc/net/bpf_jit_comp.c +0
-0}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v12 2/8 powerpc/bpf: fix alignment of long branch trampoline address
In reply to a message not in the archiveFrom: 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 | 7 +++--- arch/powerpc/net/bpf_jit_comp64.c | 7 +++--- 4 files changed, 48 insertions(+), 15 deletions(-)
arch/powerpc/net/bpf_jit.h +4 -3
@@ -221,10 +221,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 -3
@@ -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 */
@@ -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 +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.55.0
[email protected]
AI reviewRe: [PATCH v12 2/8] powerpc/bpf: fix alignment of long branch trampoline address
This isn't a bug, but could the changelog name the concrete failure mode? The actual mechanism is that bpf_arch_text_poke() patches this field via patch_ulong() which returns -EINVAL unless…
Quoted 3 lines
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 isn't a bug, but could the changelog name the concrete failure mode? The actual mechanism is that bpf_arch_text_poke() patches this field via patch_ulong() which returns -EINVAL unless IS_ALIGNED(addr, sizeof(unsigned long)), so attaching a bpf trampoline whose target is out of branch range fails when the field lands at a 4-mod-8 address.
The phrase "for memory compatibility when content loaded to a register" is vague - powerpc 'ld' does not require 8-byte alignment; the alignment requirement comes from patch_ulong().
Quoted 4 lines
arch/powerpc/net/bpf_jit_comp.c +0
-0
[ ... ]
Quoted 25 lines
arch/powerpc/net/bpf_jit_comp.c +16
-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 81
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 ());
Does the new comment block accurately describe the code? Looking at the comment and the implementation:
(1) Unit mismatch: The comment says "Emit a NOP here if (ctx->idx + ool_stub_sz) is odd" but ctx->idx is an instruction index while ool_stub_sz is a byte count (16 or 12). The code actually tests:
IS_ALIGNED((unsigned long)fimage + ctx->idx*4 + ool_stub_sz, SZL)
which properly multiplies ctx->idx by 4 to convert to bytes. The comment's stated parity expression doesn't match what the code performs.
(2) Alignment assumption contradiction: The comment states dummy_tramp_addr "lands at an even instruction offset (== 8-byte aligned from an 8-byte aligned base)", but the preceding sentence explicitly says "The fimage can be non 8-byte aligned". In fact, bpf_jit_binary_pack_alloc() is called with alignment 4 in bpf_int_jit_compile(), and the code correctly folds the real fimage address into the IS_ALIGNED() check rather than assuming an aligned base.
(3) "In the final pass" is inaccurate: the alignment decision is taken in every codegen pass where image != NULL. Looking at the loop in bpf_jit_comp.c starting around bpf_jit_build_prologue(), 2-3 such passes run, not just the final one.
Also, "the image buffer have 4 spare bytes" is ungrammatical and understates the case: a fentry-stub block is also emitted mid-body by bpf_jit_emit_exit_insn()'s fallback path, so a program can contain several stub sites and therefore several spare NOP slots.
Note: the immediate follow-up commit 1525fbfebfe9 ("powerpc/bpf: Move out dummy_tramp_addr after Long branch stub") deletes exactly this wording and replaces the 16:12 constant with 44:40, which suggests the description was known to be confusing.
---
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/32100929603
PATCH v12 3/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 ;
@@ -1313,6 +1312,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>
@@ -1320,14 +1320,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. *
@@ -1429,10 +1429,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.55.0
[email protected]
AI reviewRe: [PATCH v12 3/8] powerpc/bpf: Move out dummy_tramp_addr after Long branch stub
Does this patch fix a bug in d243b62b7bd3?
Quoted 1 line
Fixes d243b62b7bd3 ("powerpc64/bpf: Add support for bpf trampolines")
Does this patch fix a bug in d243b62b7bd3?
The commit message describes the change as moving the dummy_tramp_addr literal from the top of the long branch stub to the bottom so a disassembler can walk through the stub instructions without hitting embedded data. The message states "the actual program layout is never altered. Hence no effect on overall program size."
The 8-byte alignment invariant the changelog mentions was already established by the immediately preceding commit in the series, 206e0a244d70 ("powerpc/bpf: fix alignment of long branch trampoline address"), which carries the same Fixes: tag plus Cc: stable. Before this patch the check already ensured alignment:
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());where 16 is 4 OOL-stub instructions and 12 is 3, with the .long immediately following them. This patch re-derives the same constant for the new position (44/40) but does not repair a misalignment.
Keeping the Fixes: tag will feed this patch to AUTOSEL and stable even though it appears to be a readability improvement that textually depends on 206e0a244d70 (the reworked comment and constant hunks will not apply cleanly without it). The absence of Cc: stable here - present on both predecessor commits in the series - suggests this patch was not intended for backport either.
Should the Fixes: tag be dropped, or does it repair a specific observable failure beyond the disassembler presentation described in the changelog?
---
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/32100929603
PATCH v12 4/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
@@ -195,6 +195,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
@@ -786,7 +786,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.55.0
PATCH v12 5/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.55.0
[email protected]
AI reviewRe: [PATCH v12 5/8] powerpc64/bpf: fix percpu private stack leak on JIT failure
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Pre-existing error handling flaw in bpf_jit_free() leads to a double-free and use-after-free of the BPF binary pack if bpf_jit_binary_pack_finalize() fails.
- [High] The exact same per-CPU private stack leak fixed in this patch for PowerPC also exists in the ARM64 BPF JIT compiler.
--
--- Patch [5]: [PATCH v12 5/8] powerpc64/bpf: fix percpu private stack leak on JIT failure --- commit 7f3490460ab3b86fd0696381ed6fb1a49e89ac48
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 407
Quoted 10 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) {
[Severity: High]
This isn't a bug introduced by this patch, but does the arm64 BPF JIT have the exact same leak this patch fixes?
Looking at bpf_int_jit_compile() in arch/arm64/net/bpf_jit_comp.c:
out_off:
if (!ro_header && priv_stack_ptr) {
free_percpu(priv_stack_ptr);
prog->aux->priv_stack_ptr = NULL;
}If JIT compilation fails late (e.g., validate_ctx fails), it jumps to the out_free_hdr block which frees the header but doesn't set ro_header to NULL. Execution then falls through to out_off. Since ro_header is non-NULL, it skips freeing the stack. bpf_jit_free() also skips it because prog->jited is 0.
Should the arm64 code also be updated to use !prog->jited?
[Severity: High]
This is a pre-existing issue, but can ignoring the return value of bpf_jit_binary_pack_finalize() in bpf_jit_free() cause a double-free?
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);
// ...
}
hdr = bpf_jit_binary_pack_hdr(fp);
bpf_jit_binary_pack_free(hdr, NULL);If bpf_jit_binary_pack_finalize() fails, it frees ro_header and returns an error:
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);
}
...
}Because bpf_jit_free() ignores this return value, it immediately re-fetches the freed header and calls bpf_jit_binary_pack_free() again.
Does this lead to a use-after-free read of the garbage size and a double free of the pack?
Quoted 3 lines
arch/powerpc/net/bpf_jit_comp.c +0
-0fp->aux->priv_stack_ptr =NULL ;free_percpu (priv_stack_ptr); }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
PATCH v12 6/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.55.0
[email protected]
AI reviewRe: [PATCH v12 6/8] selftests/bpf: Fixing powerpc JIT disassembly failure
This isn't a bug, but could the subject use the imperative ('Fix ...' instead of 'Fixing ...'), and could the in-code comment and the changelog agree on why the trailing bytes are dropped - is the…
Quoted 10 lines
selftests/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.
This isn't a bug, but could the subject use the imperative ('Fix ...' instead of 'Fixing ...'), and could the in-code comment and the changelog agree on why the trailing bytes are dropped - is the hazard that they cannot be decoded, or that they can be decoded into a plausible-looking wrong instruction?
Quoted 16 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +2
-0@@ -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) {
[ ... ]
Quoted 7 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +3
-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,
tools/testing/selftests/bpf/jit_disasm_helpers.c int get_jited_program_text(int fd, char *text, size_t text_sz) · line 242
Quoted 10 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +10
-0+* 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); + }
This isn't a bug, but would #if defined(__powerpc64__) / #elif defined(__powerpc__) work here, matching how get_current_arch() in test_loader.c already detects the arch, and avoiding a second LLVMGetDefaultTargetTriple()/LLVMDisposeMessage() pair alongside the one in disasm_one_func()?
Quoted 5 lines
tools/testing/selftests/bpf/jit_disasm_helpers.c +2
-1+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]; }
This isn't a bug, but since 32-bit powerpc has no __arch_* tag and get_current_arch() has no case for it yet, would it be clearer to add the 4-byte arm together with the 32-bit enablement rather than ahead of it?
---
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/32100929603
PATCH v12 7/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.55.0
PATCH v12 8/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:
# ./test_progs -t verifier_tailcall
#618/1 verifier_tailcall/invalid map type for tail call:OK
#618/2 verifier_tailcall/invalid map type for tail call @unpriv:OK
#618 verifier_tailcall:OK
#619/1 verifier_tailcall_jit/main:OK
#619 verifier_tailcall_jit:OK
Summary: 2/3 PASSED, 0 SKIPPED, 0 FAILED
--- .../bpf/progs/verifier_tailcall_jit.c | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+)
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +76 -0
@@ -91,6 +91,82 @@ __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.55.0
[email protected]
AI reviewRe: [PATCH v12 8/8] selftests/bpf: Add tailcall verifier selftest for powerpc64
Does 'mtctr 12' work on ELF ABIv1 configurations?
Quoted 20 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +12
-0@@ -91,6 +91,82 @@ __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 +*
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c __jited(" popq %rax") · line 115
Quoted 10 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +10
-0+* 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" )
Does 'mtctr 12' work on ELF ABIv1 configurations?
Looking at bpf_jit_emit_func_call_rel(), the JIT only loads the target into r12 on ELFv2; on CONFIG_PPC64_ELF_ABI_V1 it materializes the function-descriptor address into TMP_REG_2 (r10), then does 'ld 9, 0(10)' and 'mtctr 9', so the CTR load reads 'mtctr 9' there:
arch/powerpc/net/bpf_jit_comp64.c:bpf_jit_emit_func_call_rel() {
if (PPC_ELF_ABI_V1) {
...
EMIT(PPC_RAW_LD(_R9, TMP_REG_2, 0)); /* ld 9, 0(10) */
EMIT(PPC_RAW_MTCTR(_R9)); /* mtctr 9 */
} else {
...
EMIT(PPC_RAW_MTCTR(_R12)); /* mtctr 12 */
}
}test_loader.c:get_current_arch() returns ARCH_POWERPC64 for any __powerpc64__ build regardless of ABI, so a big-endian ABIv1 kernel would run this block and see a mismatch.
If ABIv1 builds are out of scope for this test, would wildcarding the CTR register make the line ABI-agnostic?
Quoted 4 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +4
-0+__jited(" bctrl") +__jited(" mr 8, 3") +__jited(" li 8, 0") +__jited (" addi 1, 1, {{.*}}" )
[ ... ]
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c __jited(" popq %rax") · line 135
Quoted 10 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +10
-0+/* 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)" )
Should the '/* tail call prologue for subprogram */' comment end here?
The comment heading covers the next 15 lines, but only the first 6 lines (ld 10,0(1) through std 9,-8(1)) come from the is_subprog branch of bpf_jit_build_prologue(). The lines that follow are:
- Lines 136-140 (lis / sldi / oris / ori / li) are ordinary body instructions for 'r2 = jmp_table ll' and 'r3 = 0'
- Lines 141-144 (lwz / slwi / cmplw / bf 0) are already the first block of bpf_jit_emit_tail_call() - the 'index >= array->map.max_entries' check - which sits above the '/* bpf_tail_call implementation */' heading
A reader using the comment to map expectations onto JIT functions would be misdirected for 9 of the 15 lines.
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c __jited(" popq %rax") · line 139
Quoted 4 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +4
-0+__jited(" lis {{.*}}, {{.*}}") +__jited(" sldi {{.*}}, {{.*}}, 32") +__jited(" oris {{.*}}, {{.*}}, {{.*}}") +__jited (" ori {{.*}}, {{.*}}, {{.*}}" )
Does this fixed four-instruction sequence match what PPC_LI64 actually emits?
These lines hardcode a fixed expansion (lis / sldi / oris / ori) for the 'r2 = jmp_table ll' map-pointer materialization in sub(), but the ppc64 JIT emits PPC_LI64() there, whose length and shape depend on the runtime value of the pointer:
arch/powerpc/net/bpf_jit.h:PPC_LI64(d, i) {
if (!((uintptr_t)(i) & 0xffff800000000000ULL))
EMIT(PPC_RAW_LI(d, ((uintptr_t)(i) >> 32) & 0xffff));
else {
EMIT(PPC_RAW_LIS(d, ((uintptr_t)(i) >> 48)));
if ((uintptr_t)(i) & 0x0000ffff00000000ULL)
EMIT(PPC_RAW_ORI(d, d, ((uintptr_t)(i) >> 32) & 0xffff));
}
EMIT(PPC_RAW_SLDI(d, d, 32));
if ((uintptr_t)(i) & 0x00000000ffff0000ULL)
EMIT(PPC_RAW_ORIS(d, d, ((uintptr_t)(i) >> 16) & 0xffff));
if ((uintptr_t)(i) & 0x000000000000ffffULL)
EMIT(PPC_RAW_ORI(d, d, (uintptr_t)(i) & 0xffff));
}The immediate here is the struct bpf_map * returned by __bpf_map_area_alloc() (kernel/bpf/syscall.c:372), which for a small map takes the kmalloc_node() path and therefore lives in the linear map at 0xc000000000000000 | pa.
Bits 47:32 of that address are pa[47:32]. On any ppc64 machine where the allocation lands at a physical address >= 4 GiB - routine on a box with more than 4 GiB of RAM - PPC_LI64 emits an extra 'ori d, d, <pa[47:32]>' between the lis and the sldi.
Because consecutive non-"..." __jited() patterns are pushed with on_next_line = true (test_loader.c:push_disasm_msg), validate_msgs() enforces prev_match->line + 1 == match->line and the test fails with 'expected next match: "\tsldi ..." ... WRONG LINE'. Conversely, an allocation that happens to be 64 KiB aligned drops the trailing 'ori' and the line-139 pattern fails to match at all.
Would the address-dependent part of this sequence need the same '...' treatment already used for the call-target materialization in main(), for example keeping 'lis' and the terminal 'li {{.*}}, 0' as anchors and wildcarding what is between them?
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c __jited(" popq %rax") · line 146
Quoted 7 lines
tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c +7
-0+__jited(" li {{.*}}, 0") +__jited(" lwz 9, {{.*}}({{.*}})") +__jited(" slwi {{.*}}, {{.*}}, 0") +__jited(" cmplw {{.*}}, 9") +__jited (" bf 0, {{.*}}" ) +/* bpf_tail_call implementation */ +__jited (" ld 9, -8(1)" )
[ ... ]
---
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/32100929603