Re: [PATCH bpf-next v1 04/14] bpf, riscv: Convert struct_ops arena arguments in the trampoline

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

Pre-existing issues:
- [High] Unaligned memory access in JIT instruction patching
--

--- Patch [4]: [PATCH bpf-next v1 04/14] bpf, riscv: Convert struct_ops arena arguments in the trampoline ---
commit 0283a3765a698e09bc666805ea8cb746cf58f299
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf, riscv: Convert struct_ops arena arguments in the trampoline

This commit converts struct_ops callback arena arguments into the arena pointer
form when copied into the BPF context. It walks the function model by argument,
adjusts the pointers via bpf_tramp_arena_base(), and handles nullable arguments.

> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index c97d13a3eae4e..8db992a285f6a 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
[ ... ]
> @@ -888,20 +888,75 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,
>  	return ret;
>  }
>  
> -static void store_args(int nr_arg_slots, int args_off, int stack_args_off,
> +/*
> + * Convert an arena kernel address into the arena pointer form on its way
> + * into the BPF ctx, dst = (u32)(src - kern_vm_start). A nullable arg
> + * preserves NULL, tested on the full 64-bit kernel pointer. The final
> + * zero-extension makes the stored value satisfy the JIT invariant for arena
> + * pointer registers.
> + */
> +static void emit_arena_arg_conv(u8 dst, u8 src, bool nullable, u8 base,
> +				struct rv_jit_context *ctx)
> +{
> +	int branch_off = 0;
> +
> +	if (nullable) {
> +		if (dst != src)
> +			emit_mv(dst, src, ctx);
> +		branch_off = ctx->ninsns;
> +		/* Patched below once the variable-length conversion is emitted. */
> +		emit(rv_nop(), ctx);
> +		src = dst;
> +	}
> +
> +	emit_sub(dst, src, base, ctx);
> +	emit_zextw(dst, dst, ctx);
> +
> +	if (nullable && ctx->insns) {
> +		u32 insn = rv_beq(dst, RV_REG_ZERO, ctx->ninsns - branch_off);
> +
> +		*(u32 *)(ctx->insns + branch_off) = insn;

[Severity: High]
This is a pre-existing issue, but does this direct 32-bit store cause an
unaligned memory access trap?

If earlier instructions in the JIT context use 16-bit compressed instructions,
ctx->ninsns (and therefore branch_off) becomes an odd number. In that case,
ctx->insns + branch_off points to a 2-byte aligned address, not a 4-byte
aligned one.

On RISC-V hardware that does not support unaligned memory accesses, casting an
unaligned u16 pointer to a u32 pointer and performing a 32-bit store triggers
a CPU exception.

Could this be safely written as two sequential 16-bit writes to avoid the trap?

> +	}
> +}

[ ... ]

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