Re: [PATCH v6 5/6] riscv, bpf: emit LR/SC loops when Zaamo is disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] riscv, bpf: missing full fence for fetch/xchg LR/SC in
  emit_atomic_lrsc()
--

commit a367c50f4df6ecfbc1363d12bbca1b28cecbf767
Author: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

riscv, bpf: emit LR/SC loops when Zaamo is disabled

This commit updates the RV32 and RV64 BPF JITs to emit LR/SC loops for
BPF atomic read-modify-write operations when CONFIG_RISCV_ISA_ZAAMO is
disabled, and adds the second exception table entry needed when an arena
access faults on the sc instruction.

> diff --git a/arch/riscv/net/bpf_jit_comp32.c b/arch/riscv/net/bpf_jit_comp32.c
> index a9e0bd5cc81d2..ea1779a236341 100644
> --- a/arch/riscv/net/bpf_jit_comp32.c
> +++ b/arch/riscv/net/bpf_jit_comp32.c
> @@ -874,11 +874,67 @@ static int emit_load_r64(const s8 *dst, const s8 *src, s16 off,
>  	return 0;
>  }
>  
> -static int emit_bpf_atomic(s8 dst, const s8 *src, const s8 *rs,
> -			   struct rv_jit_context *ctx,
> -			   const struct bpf_insn *insn)
> +/*
> + * Read-modify-write via lr/sc for kernels built without Zaamo. T0 holds
> + * the address. T1/T2 are not BPF registers; the address was copied out of
> + * lo(rd) before this is called, so clobbering T2 is fine.
> + */
> +static int emit_bpf_atomic_lrsc(s8 dst, const s8 *rs, s32 imm,
> +				struct rv_jit_context *ctx)
> +{
> +	bool is_fetch = (imm & BPF_FETCH) || (imm == BPF_XCHG);
> +	bool xchg = imm == BPF_XCHG;
> +	u8 aq = is_fetch ? 1 : 0;
> +	u8 rl = is_fetch ? 1 : 0;
> +	u8 old = RV_REG_T1;
> +	u8 tmp = RV_REG_T2;
> +	int lr_off, jmp_off;
> +	s32 op = xchg ? BPF_XCHG : imm & ~BPF_FETCH;
> +
> +	switch (op) {
> +	case BPF_ADD:
> +	case BPF_AND:
> +	case BPF_OR:
> +	case BPF_XOR:
> +	case BPF_XCHG:
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	lr_off = ctx->ninsns;
> +	emit(rv_lr_w(old, 0, dst, aq, rl), ctx);
> +
> +	switch (op) {
> +	case BPF_ADD:
> +		emit(rv_add(tmp, old, lo(rs)), ctx);
> +		break;
> +	case BPF_AND:
> +		emit(rv_and(tmp, old, lo(rs)), ctx);
> +		break;
> +	case BPF_OR:
> +		emit(rv_or(tmp, old, lo(rs)), ctx);
> +		break;
> +	case BPF_XOR:
> +		emit(rv_xor(tmp, old, lo(rs)), ctx);
> +		break;
> +	}
> +
> +	if (xchg)
> +		emit(rv_sc_w(tmp, lo(rs), dst, aq, rl), ctx);
> +	else
> +		emit(rv_sc_w(tmp, tmp, dst, aq, rl), ctx);
> +
> +	jmp_off = ninsns_rvoff(lr_off - ctx->ninsns);
> +	emit(rv_bne(tmp, RV_REG_ZERO, jmp_off >> 1), ctx);
> +	if (is_fetch)
> +		emit(rv_addi(lo(rs), old, 0), ctx);
> +	return 0;
> +}

[ ... ]

> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 01fe66774f029..94e4ddc6d965c 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -573,6 +573,79 @@ static int emit_atomic_ld_st(u8 rd, u8 rs, const struct bpf_insn *insn,
>  	return 0;
>  }
>  
> +/*
> + * Read-modify-write via lr/sc for kernels built without Zaamo, matching
> + * the Zalrsc sequences in arch/riscv/include/asm/atomic.h. A relaxed op
> + * discards the old value. A fetch op (including XCHG) uses .aqrl and
> + * writes that value back to val. T2/T3 are not BPF registers; addr may
> + * already be T1.
> + */
> +static void emit_atomic_lrsc(u8 addr, u8 val, bool is64, s32 imm,
> +			     struct rv_jit_context *ctx)
> +{
> +	bool fetch = imm & BPF_FETCH;
> +	u8 aq = fetch ? 1 : 0;
> +	u8 rl = fetch ? 1 : 0;
> +	u8 old = RV_REG_T2;
> +	u8 tmp = RV_REG_T3;
> +	int lr_off, jmp_off;
> +	s32 op = imm == BPF_XCHG ? BPF_XCHG : imm & ~BPF_FETCH;
> +
> +	lr_off = ctx->ninsns;
> +	ctx->ex_insn_off = ctx->ninsns;
> +	emit(is64 ? rv_lr_d(old, 0, addr, aq, rl) :
> +	     rv_lr_w(old, 0, addr, aq, rl), ctx);
> +
> +	if (op != BPF_XCHG) {
> +		if (!is64 && op == BPF_ADD)
> +			emit(rv_addw(tmp, old, val), ctx);
> +		else if (op == BPF_ADD)
> +			emit_add(tmp, old, val, ctx);
> +		else if (op == BPF_AND)
> +			emit_and(tmp, old, val, ctx);
> +		else if (op == BPF_OR)
> +			emit_or(tmp, old, val, ctx);
> +		else if (op == BPF_XOR)
> +			emit_xor(tmp, old, val, ctx);
> +
> +		ctx->ex_sc_off = ctx->ninsns;
> +		emit(is64 ? rv_sc_d(tmp, tmp, addr, aq, rl) :
> +		     rv_sc_w(tmp, tmp, addr, aq, rl), ctx);
> +	} else {
> +		ctx->ex_sc_off = ctx->ninsns;
> +		emit(is64 ? rv_sc_d(tmp, val, addr, aq, rl) :
> +		     rv_sc_w(tmp, val, addr, aq, rl), ctx);
> +	}
> +
> +	/* Branch offset is relative to this instruction, back to the lr. */
> +	jmp_off = ninsns_rvoff(lr_off - ctx->ninsns);
> +	emit(rv_bne(tmp, RV_REG_ZERO, jmp_off >> 1), ctx);
> +
> +	if (fetch) {
> +		emit_mv(val, old, ctx);
> +		if (!is64)
> +			emit_zextw(val, val, ctx);
> +	}
> +	ctx->ex_jmp_off = ctx->ninsns;
> +}

[Severity: Medium]
Does using .aqrl on lr and sc without a trailing fence provide the full
memory barrier required by BPF_FETCH and BPF_XCHG in
emit_bpf_atomic_lrsc() and emit_atomic_lrsc()?

Under RVWMO, lr.aqrl is a load whose .rl bit only orders prior RCsc
accesses, and sc.aqrl is a store whose .aq bit only orders subsequent RCsc
accesses.  A plain store before lr.aqrl and a plain load after sc.aqrl can
therefore be reordered across the LR/SC loop.

The Zalrsc sequence in arch/riscv/include/asm/atomic.h and emit_cmpxchg()
in arch/riscv/net/bpf_jit.h instead emit a plain lr (aq=0, rl=0), sc.rl
(aq=0, rl=1), and a full barrier after the retry branch:

arch/riscv/include/asm/atomic.h:ATOMIC_FETCH_OP() {
	__asm__ __volatile__ (
		"1:	lr." #asm_type " %1, %0\n"
		"	" #asm_op " %2, %1, %3\n"
		"	sc." #asm_type ".rl %2, %2, %0\n"
		"	bnez %2, 1b\n"
		RISCV_FULL_BARRIER
}

Both emit_bpf_atomic_lrsc() and emit_atomic_lrsc() should use plain lr,
sc.rl for fetch/xchg, and emit_fence_rw_rw(ctx) after the bne retry
branch.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-lrsc-only-v6-0-bafc3cc0fcae@mobileye.com?part=5