Re: [PATCH 2/5] LoongArch: BPF: Add private stack support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Per-CPU private stack memory leak upon JIT compilation failure.
- [Low] Undeclared identifier 'LOONGARCH_GPR_U0' causes build failure.
- [Low] Typo in the private stack guard check error message (`%sx` instead of `%s`).

Pre-existing issues:
- [Critical] Incorrect BPF_TAIL_CALL_CNT_PTR_STACK_OFF offset calculation when arena or exception boundary features are used.
--

commit ec3426d8bc4121c18182b9bd92e8adc7fa663dc5
Author: George Guo <guodongtai@kylinos.cn>

LoongArch: BPF: Add private stack support

Support per-program private stacks to reduce kernel stack pressure for deep
or recursive tracing programs. The stack depth is computed and allocated
per-CPU with guard regions. If a private stack is used, the BPF frame
pointer is pointed into this region instead of the kernel stack.

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -18,8 +18,13 @@
>  
>  #define REG_TCC		LOONGARCH_GPR_A6
>  #define REG_ARENA	LOONGARCH_GPR_S6 /* For storing arena_vm_start */
> +#define REG_PRIV_SP	LOONGARCH_GPR_S5 /* For storing the private stack pointer */
>  #define BPF_TAIL_CALL_CNT_PTR_STACK_OFF(stack) (round_up(stack, 16) - 80)

[Severity: Critical]
This isn't a bug introduced by this patch, but is the tail call count
offset calculation correct when arena or exception boundary features are
enabled? When ctx->arena_vm_start or is_exception_prog is true, 8 additional
bytes are reserved on the stack, which shifts the actual location of TCC_PTR.
Because the macro doesn't account for this shift, will tail calls load the
TCC counter value instead of the pointer address, causing a kernel panic
when it's dereferenced?

[ ... ]

> +static void emit_percpu_ptr(struct jit_ctx *ctx, u8 dst, void __percpu *ptr)
> +{
> +	move_imm(ctx, dst, (__force long)ptr, false);
> +#ifdef CONFIG_SMP
> +	/* dst += __my_cpu_offset, held in $r21 */
> +	emit_insn(ctx, addd, dst, dst, LOONGARCH_GPR_U0);
> +#endif
> +}

[Severity: Low]
Does LOONGARCH_GPR_U0 exist in enum loongarch_gpr? It appears missing from
the loongarch include headers, which would cause a build failure when
CONFIG_SMP is enabled.

[ ... ]

> +static void priv_stack_check_guard(void __percpu *priv_stack_ptr, int alloc_size,
> +				   struct bpf_prog *prog)
> +{
[ ... ]
> +		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(prog));

[Severity: Low]
Is the "%sx" format specifier intentional here? This looks like a typo that
would append a literal 'x' to the end of the program name in the error log.

[ ... ]

>  	if (!prog->is_func || extra_pass) {
>  		if (extra_pass && ctx.idx != jit_data->ctx.idx) {
>  			pr_err_once("multi-func JIT bug %d != %d\n",
>  				    ctx.idx, jit_data->ctx.idx);
>  			goto out_free;
>  		}
[ ... ]
>  out_offset:
> +		/*
> +		 * A NULL ro_header here means the JIT failed, so release the
> +		 * private stack that was allocated above; on success the
> +		 * program keeps it until bpf_jit_free().
> +		 */
> +		if (!ro_header && priv_stack_ptr) {
> +			free_percpu(priv_stack_ptr);
> +			prog->aux->priv_stack_ptr = NULL;
> +		}
>  		kvfree(ctx.offset);
> +out_priv_stack:
>  		kfree(jit_data);
>  		prog->aux->jit_data = NULL;
>  	}
>  
>  	return prog;
>  
>  out_free:
>  	if (extra_pass) {
>  		prog->bpf_func = NULL;
>  		prog->jited = 0;
>  		prog->jited_len = 0;
>  	}
>  
>  	if (header) {
>  		bpf_arch_text_copy(&ro_header->size, &header->size, sizeof(header->size));
>  		bpf_jit_binary_pack_free(ro_header, header);
>  	}
>  	goto out_offset;
>  }

[Severity: High]
Does this code leak the private stack allocation when JIT compilation fails?
When jumping to out_free, ro_header is freed but the local pointer is not set
to NULL. 

Because ro_header is still non-NULL when execution falls through to
out_offset, the free_percpu() cleanup block is skipped. Furthermore, because
prog->jited is set to 0, bpf_jit_free() will also skip freeing it, resulting
in a permanent leak of the per-CPU allocation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618033809.98253-1-dongtai.guo@linux.dev?part=2