Re: [PATCH bpf-next v1 08/14] bpf, loongarch: JIT arena kfunc argument rebasing

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

Pre-existing issues:
- [High] The `insn_is_cast_user` JIT implementation on LoongArch fails to preserve the nullable zero (NULL) due to an off-by-one error in the branch offset.
--

--- Patch [8]: [PATCH bpf-next v1 08/14] bpf, loongarch: JIT arena kfunc argument rebasing ---
commit dafba7b807dd9770b1b1fe373622d157b3fde2ab
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf, loongarch: JIT arena kfunc argument rebasing

Kfunc parameters marked with BTF_FMODEL_ARENA_ARG carry a 32-bit arena
offset in the BPF register, while the native kfunc expects a directly
dereferenceable kernel address. This patch implements the JIT conversion
for LoongArch to correctly add the arena base address to these offsets.

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index d193293a0fd25..33cabaa7353f9 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
[ ... ]
> @@ -1195,9 +1200,22 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>  
>  			for (i = 0; i < m->nr_args; i++) {
>  				u8 reg = regmap[BPF_REG_1 + i];
> -				bool sign = m->arg_flags[i] & BTF_FMODEL_SIGNED_ARG;
> -
> -				emit_abi_ext(ctx, reg, m->arg_size[i], sign);
> +				u8 flags = m->arg_flags[i];
> +
> +				if (flags & BTF_FMODEL_ARENA_ARG) {
> +					if (WARN_ON_ONCE(!ctx->arena_vm_start))
> +						return -EINVAL;
> +
> +					/* rN = kern_vm_start + (u32)rN */
> +					emit_zext_32(ctx, reg, true);
> +					if (flags & BTF_FMODEL_NULLABLE_ARG)
> +						emit_insn(ctx, beq, reg, LOONGARCH_GPR_ZERO, 2);

[Severity: High]
This isn't a bug introduced by this patch, but seeing the correct offset of
2 used here to skip the subsequent addition brings up a question about the
pre-existing arena pointer cast implementation.

Does the pre-existing code for insn_is_cast_user in build_insn() have an
off-by-one error?

When handling the cast instruction, the code does:

arch/loongarch/net/bpf_jit.c:build_insn() {
    ...
    emit_insn(ctx, beq, t1, LOONGARCH_GPR_ZERO, 1);
    emit_insn(ctx, or, t1, dst, t1);
    move_reg(ctx, dst, t1);
    ...
}

Because the branch offset is 1 instead of 2, won't it branch to PC + 4
and execute the or instruction instead of skipping it? If so, a NULL
pointer would be incorrectly converted to the arena base address instead
of remaining NULL.

> +					emit_insn(ctx, addd, reg, reg, REG_ARENA);
> +					continue;
> +				}
> +
> +				emit_abi_ext(ctx, reg, m->arg_size[i],
> +					     flags & BTF_FMODEL_SIGNED_ARG);
>  			}
>  		}

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