Re: [PATCH bpf-next] bpf, riscv: Register extable entries at probe insn call sites

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] BPF_PROBE_ATOMIC cmpxchg on RISC-V without ZACAS lacks exception table entries, leading to an unhandled page fault and kernel panic for unmapped arena accesses.
--

commit 77ab37f77b219155ec1dc7c551ef131f6c642039
Author: Chen Pei <cp0613@linux.alibaba.com>
bpf, riscv: Register extable entries at probe insn call sites

This commit removes the mode gate from add_exception_handler() and explicitly
registers exception handlers only at probe instruction call sites. It aims
to prevent collisions between probe mode values and legitimate instruction
encodings.

> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index e7378be171a9f..726c0169fc60f 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
[ ... ]
> @@ -2001,7 +2007,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>  			ret = emit_atomic_rmw(rd, rs, insn, ctx);
>  
>  		/* ret can be 1 (skip-zext); extable entry still needs to be added */
> -		if (ret >= 0) {
> +		if (ret >= 0 && BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
>  			/*
>  			 * A load-acquire reads into dst_reg, and a read-modify-write
>  			 * carrying BPF_FETCH reads the old value into src_reg, or into
> @@ -2010,7 +2016,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>  			 */
>  			int load_reg = bpf_atomic_load_reg(insn);
>  
> -			ret = add_exception_handler(insn, load_reg < 0 ?
> +			ret = add_exception_handler(load_reg < 0 ?
>  					REG_DONT_CLEAR_MARKER : regmap[load_reg],
>  					ctx) ?: ret;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does emit_cmpxchg() correctly
establish an exception handler boundary when ZACAS is not enabled?

If a BPF program uses BPF_PROBE_ATOMIC with BPF_CMPXCHG on an unmapped memory
address on a RISC-V CPU without the ZACAS extension, emit_cmpxchg() falls
back to a load-reserved/store-conditional loop:

arch/riscv/net/bpf_jit.h:emit_cmpxchg() {
    ...
	if (rv_ext_enabled(ZACAS)) {
		ctx->ex_insn_off = ctx->ninsns;
		emit(is64 ? rvzacas_amocas_d(r0, rs, rd, 1, 1) :
		     rvzacas_amocas_w(r0, rs, rd, 1, 1), ctx);
		ctx->ex_jmp_off = ctx->ninsns;
        ...
		return;
	}

	if (is64)
		emit_mv(RV_REG_T2, r0, ctx);
	else
		emit_addiw(RV_REG_T2, r0, 0, ctx);
	emit(is64 ? rv_lr_d(r0, 0, rd, 0, 0) :
	     rv_lr_w(r0, 0, rd, 0, 0), ctx);
    ...
}

In the non-ZACAS path, ctx->ex_insn_off and ctx->ex_jmp_off are not updated
before emitting the faulting load-reserved instruction.

When add_exception_handler() is subsequently called here in
bpf_jit_emit_insn(), it will use stale offsets. Can this cause an unhandled
page fault and a kernel panic when accessing unmapped arena memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901120013.16104-1-cp0613@linux.alibaba.com?part=1