Re: [bpf-next PATCH 1/2] bpf, riscv: Add BPF stack arguments support for RV64 JIT

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing sign-extension for 32-bit kfunc arguments passed on the stack
--

--- Patch [1]: [bpf-next PATCH 1/2] bpf, riscv: Add BPF stack arguments support for RV64 JIT ---
commit c291f39756942d05aeab1b370b77119f4235a7f8
Author: Feng Jiang <[email protected]>

bpf, riscv: Add BPF stack arguments support for RV64 JIT

This commit adds support for passing BPF stack arguments to BPF subprograms
and kfuncs in the RV64 JIT. It implements the RISC-V ABI requirement of
putting arguments 6-8 in registers A5-A7 and arguments 9+ on the stack.

> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 6b9972b07c1b0..b3f4bf9e1e7f2 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
[ ... ]
> @@ -1815,18 +1815,39 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
[ ... ]
>  		if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>  			const struct btf_func_model *fm;
> -			int idx;
> +			int idx, nargs;
>  
>  			fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
>  			if (!fm)
>  				return -EINVAL;
>  
> -			for (idx = 0; idx < fm->nr_args; idx++) {
> +			nargs = min_t(int, fm->nr_args, MAX_BPF_FUNC_REG_ARGS);
> +			for (idx = 0; idx < nargs; idx++) {
>  				u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
>  
>  				if (fm->arg_size[idx] == sizeof(int))
>  					emit_sextw(reg, reg, ctx);
>  			}
> +
> +			/* BPF stack args -> RISC-V ABI: args 6-8 in A5-A7, 9+ at SP+0 */
> +			if (fm->nr_args > MAX_BPF_FUNC_REG_ARGS) {
> +				int n_stack = fm->nr_args - MAX_BPF_FUNC_REG_ARGS;
> +				int n_reg = min_t(int, n_stack,
> +						  RV_MAX_REG_ARGS - MAX_BPF_FUNC_REG_ARGS);
> +
> +				for (idx = 0; idx < n_reg; idx++) {
> +					int sz = fm->arg_size[MAX_BPF_FUNC_REG_ARGS + idx];
> +
> +					emit_ld(RV_REG_A5 + idx, idx * 8, RV_REG_SP, ctx);
> +					if (sz == sizeof(int))
> +						emit_sextw(RV_REG_A5 + idx, RV_REG_A5 + idx, ctx);
> +				}
> +
> +				for (idx = 0; idx < n_stack - n_reg; idx++) {
> +					emit_ld(RV_REG_T1, (n_reg + idx) * 8, RV_REG_SP, ctx);
> +					emit_sd(RV_REG_SP, idx * 8, RV_REG_T1, ctx);
> +				}

[Severity: High]
Does this loop need to sign-extend 32-bit integer arguments passed on the
stack?

The RISC-V RV64 C ABI requires scalars narrower than 64 bits to be
sign-extended to 64 bits when passed on the stack. BPF ALU32 operations
zero-extend values, so if a negative 32-bit integer is passed on the stack
(argument 9 or above), its upper 32 bits will be zero instead of ones.

The preceding loops for register arguments (both BPF_REG_1-5 and A5-A7)
check fm->arg_size[idx] == sizeof(int) and use emit_sextw(). Is a similar
check needed here before writing the argument to the stack?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1