Re: [PATCH v12 2/8] powerpc/bpf: fix alignment of long branch trampoline address

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4422 bytes --]

> 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().

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 8bee2674d4a9..caea53cde063 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c

[ ... ]

> @@ -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());

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