Re: [PATCH bpf-next 17/18] bpf, arm64: Add JIT support for stack arguments

[Puranjay Mohan <puranjay@kernel.org>]

bot+bpf-ci@kernel.org writes:

>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
>
> [ ... ]
>
>> @@ -1671,7 +1727,7 @@
>>  	}
>>  	if (src == fp) {
>>  		src_adj = ctx->priv_sp_used ? priv_sp : A64_SP;
>> -		off_adj = off + ctx->stack_size;
>> +		off_adj = off + ctx->stack_size + ctx->stack_arg_size;
>>  	} else {
>>  		src_adj = src;
>>  		off_adj = off;
>
> When priv_sp_used is true, does this offset calculation produce correct
> addresses for BPF fp-relative accesses?
>
> Looking at build_prologue:
>
> arch/arm64/net/bpf_jit_comp.c:build_prologue() {
>     ...
>     emit(A64_MOV(1, fp, priv_sp), ctx);
>     emit(A64_ADD_I(1, fp, fp, ctx->stack_size), ctx);  /* line 612 */
>
>     if (ctx->stack_size && !ctx->priv_sp_used)
>         emit(A64_SUB_I(1, A64_SP, A64_SP, ctx->stack_size), ctx);  /* line 620 */
>
>     if (ctx->stack_arg_size)
>         emit(A64_SUB_I(1, A64_SP, A64_SP, ctx->stack_arg_size), ctx);  /* line 623 */
>     ...
> }
>
> When priv_sp_used is true:
> - fp = priv_sp + stack_size (line 612)
> - main SP is not decremented by stack_size (skipped at line 620)
> - main SP is decremented by stack_arg_size (line 623-624)
>
> So the BPF program data lives on the private stack, while outgoing stack
> args live on the real SP. These are separate memory regions.
>
> In build_insn, the computed address for fp-relative loads/stores becomes:
>     addr = priv_sp + off + stack_size + stack_arg_size
>
> But the intended BPF address is:
>     addr = fp + off = priv_sp + stack_size + off
>
> The computed address is stack_arg_size bytes too high, accessing memory
> above the intended location on the private stack.

> This would read/write into the private-stack guard region (16 bytes) or
> past it when stack_arg_size > 16. Can this occur?  The verifier only
> forbids tail_calls + stack_args (verifier.c:5657), not priv_stack +
> stack_args. A kprobe BPF program that uses BPF_REG_FP and calls a kfunc
> with 9+ arguments would trigger wrong memory accesses.

This analysis is correct, will fix in the next version.

> Note that x86's bpf_int_jit_compile prevents priv_stack allocation when
> stack_arg_depth != 0 (x86/bpf_jit_comp.c:3883). The arm64 version has no
> equivalent guard.
>
> The same issue affects the ST case:
>
>> @@ -1762,7 +1823,7 @@
>>  	}
>>  	if (dst == fp) {
>>  		dst_adj = ctx->priv_sp_used ? priv_sp : A64_SP;
>> -		off_adj = off + ctx->stack_size;
>> +		off_adj = off + ctx->stack_size + ctx->stack_arg_size;
>>  	} else {
>>  		dst_adj = dst;
>>  		off_adj = off;
>
> and the STX case:
>
>> @@ -1824,7 +1890,7 @@
>>  	}
>>  	if (dst == fp) {
>>  		dst_adj = ctx->priv_sp_used ? priv_sp : A64_SP;
>> -		off_adj = off + ctx->stack_size;
>> +		off_adj = off + ctx->stack_size + ctx->stack_arg_size;
>>  	} else {
>>  		dst_adj = dst;
>>  		off_adj = off;
>
> [ ... ]
>
>> @@ -2065,6 +2131,14 @@
>>  	ctx.user_vm_start = bpf_arena_get_user_vm_start(prog->aux->arena);
>>  	ctx.arena_vm_start = bpf_arena_get_kern_vm_start(prog->aux->arena);
>>
>> +	if (prog->aux->stack_arg_depth > prog->aux->incoming_stack_arg_depth) {
>> +		u16 outgoing = prog->aux->stack_arg_depth - prog->aux->incoming_stack_arg_depth;
>> +		int nr_on_stack = outgoing / sizeof(u64) - NR_STACK_ARG_REGS;
>> +
>> +		if (nr_on_stack > 0)
>> +			ctx.stack_arg_size = round_up(nr_on_stack * sizeof(u64), 16);
>> +	}
>> +
> Alexei Starovoitov raised concerns about this logic during v1 review (Apr
> 21, 2026). He found the comparison between stack_arg_depth and
> incoming_stack_arg_depth confusing and suggested simplifying it:
>
> "I think stack_arg_depth should mean outgoing and
> incoming_stack_arg_depth should mean incoming only and it shouldn't be
> even used by JIT. That memory was allocated by caller, so to JIT this
> callee the conversion of r11+const is straightforward and no checks
> necessary. [...] I think it should really be one check based on
> stack_arg_depth."


But if you see where these feilds are defined:

+	u16 incoming_stack_arg_depth;
+	u16 stack_arg_depth; /* incoming + max outgoing */

So, if stack_arg_depth is not the outgoing stack depth, rather it is the
sum of both outgoing and incoming. We need to do stack_arg_depth - incoming_stack_arg_depth 
to find out the amount of space to reserve to passing stack arguments.

Only if stack_arg_depth meant outgoing stack arg depth only, then we
could simplify 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/24902767240