Re: [PATCH bpf-next v2 2/6] bpf, riscv: Clear fetch destination on faulting arena atomic

[Pu Lehui <pulehui@huawei.com>]

Hi Daniel,

On 2026/8/11 6:18, Daniel Borkmann wrote:
> A RMW atomic on an arena pointer is converted to BPF_PROBE_ATOMIC and
> gets an exception table entry, but that entry records no destination
> register to clear unless the instruction is a load-acquire today. That
> is right for a plain BPF_{ADD,AND,OR,XOR}, which only writes memory,
> but an RMW carrying BPF_FETCH also reads the old value into a register:
> src_reg for BPF_{ADD,AND,OR,XOR} | BPF_FETCH and BPF_XCHG, and r0 for
> BPF_CMPXCHG. emit_atomic_rmw() emits it that way, e.g.:
> 
>    [...]
>    case BPF_XCHG:
>            ctx->ex_insn_off = ctx->ninsns;
>            emit(is64 ? rv_amoswap_d(rs, rs, rd, 1, 1) :
>                 rv_amoswap_w(rs, rs, rd, 1, 1), ctx);
>    [...]
> 
> Thus, a fault over an unmapped arena page ex_handler_bpf() jumps over
> the access but leaves rs untouched, and the program resumes with
> whatever it held before the atomic instead of the 0 that every other
> BPF_PROBE_* access delivers. Fill the exception table entry in from
> bpf_atomic_load_reg(), which returns the BPF register an atomic reads
> the memory operand into or -1 when it has none. A load-acquire ends up
> with the same register it gets today, it just goes through the helper.
> Unlike x86-64 and arm64, riscv64 does not report arena violations from
> its exception handler, so there is no access direction to correct here,
> only the missing register clear.
> 
> Fixes: fb7cefabae81 ("riscv, bpf: Add support arena atomics for RV64")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Pu Lehui <pulehui@huawei.com>
> ---
>   arch/riscv/net/bpf_jit_comp64.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 6b9972b07c1b..2504df1fa111 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -1992,10 +1992,19 @@ 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)
> -			ret = add_exception_handler(insn,
> -				bpf_atomic_is_load_acq(insn) ? rd : REG_DONT_CLEAR_MARKER,
> -				ctx) ?: ret;
> +		if (ret >= 0) {
> +			/*
> +			 * A load-acquire reads into dst_reg, and a read-modify-write
> +			 * carrying BPF_FETCH reads the old value into src_reg, or into
> +			 * r0 for a BPF_CMPXCHG. Clear that register on fault, the
> +			 * remaining atomics have no destination register.
> +			 */
> +			int load_reg = bpf_atomic_load_reg(insn);
> +
> +			ret = add_exception_handler(insn, load_reg < 0 ?
> +					REG_DONT_CLEAR_MARKER : regmap[load_reg],

Felt a bit odd using regmap directly here at first, but since 
insn->dst_reg and insn->src_reg are already marked via bpf_to_rv_reg at 
the start, it looks fine.

Reviewed-by: Pu Lehui <pulehui@huawei.com>

Thanks.

> +					ctx) ?: ret;
> +		}
>   
>   		if (ret)
>   			return ret;