Re: [PATCH bpf-next 3/3] riscv, bpf: Add 32 bit atomic operations to RV32 JIT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] BPF atomic fetch operations and BPF_XCHG are emitted with relaxed memory ordering, violating the BPF memory model.
--

commit f7db41188f6daa7740f66e1c205fcef2107a88ae
Author: Kuan-Wei Chiu <visitorckw@gmail.com>

riscv, bpf: Add 32 bit atomic operations to RV32 JIT

This commit adds native support for 32-bit BPF atomic operations to the RV32
BPF JIT compiler. It maps BPF_ADD, BPF_AND, BPF_OR, BPF_XOR, and BPF_XCHG
directly to their corresponding RISC-V amo*.w instructions.

> diff --git a/arch/riscv/net/bpf_jit_comp32.c b/arch/riscv/net/bpf_jit_comp32.c
> index f8509950fed4..710c56b20f98 100644
> --- a/arch/riscv/net/bpf_jit_comp32.c
> +++ b/arch/riscv/net/bpf_jit_comp32.c
[ ... ]
> @@ -902,11 +902,40 @@ static int emit_store_r64(const s8 *dst, const s8 *src, s16 off,
>  		case BPF_MEM:
>  			emit(rv_sw(RV_REG_T0, 0, lo(rs)), ctx);
>  			break;
> -		case BPF_ATOMIC: /* Only BPF_ADD supported */
> -			emit(rv_amoadd_w(RV_REG_ZERO, lo(rs), RV_REG_T0, 0, 0),
> -			     ctx);
> +		case BPF_ATOMIC:
> +		{
> +			s8 fetch_reg = (imm & BPF_FETCH) ? lo(rs) : RV_REG_ZERO;
> +
> +			switch (imm) {
> +			case BPF_ADD:
> +			case BPF_ADD | BPF_FETCH:
> +				emit(rv_amoadd_w(fetch_reg, lo(rs), RV_REG_T0, 0, 0), ctx);
> +				break;
> +			case BPF_AND:
> +			case BPF_AND | BPF_FETCH:
> +				emit(rv_amoand_w(fetch_reg, lo(rs), RV_REG_T0, 0, 0), ctx);
> +				break;
> +			case BPF_OR:
> +			case BPF_OR | BPF_FETCH:
> +				emit(rv_amoor_w(fetch_reg, lo(rs), RV_REG_T0, 0, 0), ctx);
> +				break;
> +			case BPF_XOR:
> +			case BPF_XOR | BPF_FETCH:
> +				emit(rv_amoxor_w(fetch_reg, lo(rs), RV_REG_T0, 0, 0), ctx);
> +				break;
> +			case BPF_XCHG:
> +				emit(rv_amoswap_w(fetch_reg, lo(rs), RV_REG_T0, 0, 0), ctx);
> +				break;

According to the BPF instruction set specification, atomic operations that
return a value (like BPF_FETCH variants and BPF_XCHG) must have fully
ordered semantics and act as full memory barriers.

Since the aq (acquire) and rl (release) bits are passed as 0 to the
rv_amo*_w functions here, doesn't this emit the instructions with relaxed
memory ordering? Could this allow the CPU to reorder memory accesses around
the atomic operations and lead to data races or synchronization failures in
BPF programs?

> +			default:
> +				return -1;
> +			}
> +			if (imm & BPF_FETCH) {
> +				emit(rv_addi(hi(rs), RV_REG_ZERO, 0), ctx);
> +				bpf_put_reg64(src, rs, ctx);
> +			}
>  			break;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429123513.3477780-1-visitorckw@gmail.com?part=3