Re: [PATCH v2 1/2] powerpc64/bpf: Implement JIT support for private stack

[bot+bpf-ci@kernel.org]

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

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c

[ ... ]

> +static void priv_stack_check_guard(void __percpu *priv_stack_ptr, int alloc_size,
> +								struct bpf_prog *fp)
> +{
> +	int cpu, underflow_idx = (alloc_size - PRIV_STACK_GUARD_SZ) >> 3;
> +	u64 *stack_ptr;
> +
> +	for_each_possible_cpu(cpu) {
> +		stack_ptr = per_cpu_ptr(priv_stack_ptr, cpu);
> +		if (stack_ptr[0] != PRIV_STACK_GUARD_VAL ||
> +			stack_ptr[1] != PRIV_STACK_GUARD_VAL ||
> +			stack_ptr[underflow_idx] != PRIV_STACK_GUARD_VAL ||
> +			stack_ptr[underflow_idx + 1] != PRIV_STACK_GUARD_VAL) {
> +			pr_err("BPF private stack overflow/underflow detected for prog %sx\n",
> +			bpf_jit_get_prog_name(fp));

Should "%sx" be "%s"? The trailing 'x' would be appended to the
program name in the error output.

> +			break;
> +		}
> +	}
> +}

[ ... ]

> +	priv_stack_ptr = fp->aux->priv_stack_ptr;
> +	if (!priv_stack_ptr && fp->aux->jits_use_priv_stack) {

[ ... ]

> +		priv_stack_ptr = __alloc_percpu_gfp(priv_stack_alloc_size, 16, GFP_KERNEL);
> +		if (!priv_stack_ptr) {
> +			fp = org_fp;
> +			goto out_priv_stack;
> +		}

[ ... ]

>  out_addrs:
> +		if (!image && priv_stack_ptr) {
> +			fp->aux->priv_stack_ptr = NULL;
> +			free_percpu(priv_stack_ptr);
> +		}
> +out_priv_stack:
>  		kfree(addrs);

When __alloc_percpu_gfp() fails above, goto out_priv_stack jumps
here. At that point, addrs is declared as:

    u32 *addrs;

without initialization and has not yet been assigned (the assignment
addrs = jit_data->addrs comes after the goto). This passes a garbage
pointer to kfree().

The x86 JIT has kvfree(addrs) at the out_addrs label (before
out_priv_stack), so the goto to out_priv_stack skips it. Should the
label ordering here be adjusted similarly, or should addrs be
initialized to NULL?

> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c

[ ... ]

>  		EMIT(PPC_RAW_STDU(_R1, _R1,
> -				-(bpf_jit_stack_size(ctx) + ctx->stack_size)));
> +			-(bpf_jit_stack_size(ctx) + (ctx->priv_sp ? 0 : ctx->stack_size))));
>  	}

The STDU frame size is now bpf_jit_stack_size(ctx) when priv_sp is
set, excluding ctx->stack_size. However, bpf_jit_stack_offsetof()
still computes NVR save offsets including ctx->stack_size:

    bpf_jit_stack_offsetof() {
        ...
        return (bpf_has_stack_frame(ctx) ?
            (frame_nvr_size + ctx->stack_size) : 0)
                - (8 * (32 - reg)) - BPF_PPC_TAILCALL;
    }

Similarly, bpf_jit_stack_local() returns
STACK_FRAME_MIN_SIZE + ctx->stack_size.

Since the STDU frame no longer reserves space for ctx->stack_size
when using private stack, these offsets point ctx->stack_size bytes
beyond the allocated frame. This would cause NVR saves in the
prologue and restores in the epilogue to write past the frame
boundary into the caller's stack.

The x86 JIT handles this by zeroing stack_depth in do_jit() when
private stack is active, which keeps all offset calculations
consistent. Would a similar approach work here, either zeroing
ctx->stack_size after setting up priv_sp, or updating the offset
functions to account for it?

[ ... ]

>  	/* Tear down our stack frame */
>  	if (bpf_has_stack_frame(ctx)) {
> -		EMIT(PPC_RAW_ADDI(_R1, _R1, bpf_jit_stack_size(ctx) + ctx->stack_size));
> +		EMIT(PPC_RAW_ADDI(_R1, _R1,
> +			bpf_jit_stack_size(ctx) + (ctx->priv_sp ? 0 : ctx->stack_size)));

Same issue here -- the ADDI restore is correct, but the NVR restores
emitted by bpf_jit_emit_common_epilogue() use
bpf_jit_stack_offsetof(), which still includes ctx->stack_size in
the offset and would load from the wrong location.


---
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/22393518231

AI-authorship-score: low
AI-authorship-explanation: The code shows typical human porting errors and organic review iteration across v1/v2, inconsistent with AI-generated code patterns.
issues-found: 3
issue-severity-score: high
issue-severity-explanation: Stack corruption from mismatched NVR save offsets affects every PPC64 BPF program using private stack, and kfree of an uninitialized pointer can crash or corrupt the slab allocator under memory pressure.