Re: [PATCH bpf-next v1 3/8] bpf: Introduce load-acquire and store-release instructions

[Eduard Zingerman <eddyz87@gmail.com>]

On Sat, 2025-01-25 at 02:18 +0000, Peilin Ye wrote:
> Introduce BPF instructions with load-acquire and store-release
> semantics, as discussed in [1].  The following new flags are defined:
> 
>   BPF_ATOMIC_LOAD         0x10
>   BPF_ATOMIC_STORE        0x20
>   BPF_ATOMIC_TYPE(imm)    ((imm) & 0xf0)
> 
>   BPF_RELAXED        0x0
>   BPF_ACQUIRE        0x1
>   BPF_RELEASE        0x2
>   BPF_ACQ_REL        0x3
>   BPF_SEQ_CST        0x4
> 
>   BPF_LOAD_ACQ       (BPF_ATOMIC_LOAD | BPF_ACQUIRE)
>   BPF_STORE_REL      (BPF_ATOMIC_STORE | BPF_RELEASE)
> 
> A "load-acquire" is a BPF_STX | BPF_ATOMIC instruction with the 'imm'
> field set to BPF_LOAD_ACQ (0x11).
> 
> Similarly, a "store-release" is a BPF_STX | BPF_ATOMIC instruction with
> the 'imm' field set to BPF_STORE_REL (0x22).
> 
> Unlike existing atomic operations that only support BPF_W (32-bit) and
> BPF_DW (64-bit) size modifiers, load-acquires and store-releases also
> support BPF_B (8-bit) and BPF_H (16-bit).  An 8- or 16-bit load-acquire
> zero-extends the value before writing it to a 32-bit register, just like
> ARM64 instruction LDARH and friends.
> 
> As an example, consider the following 64-bit load-acquire BPF
> instruction:
> 
>   db 10 00 00 11 00 00 00  r0 = load_acquire((u64 *)(r1 + 0x0))
> 
>   opcode (0xdb): BPF_ATOMIC | BPF_DW | BPF_STX
>   imm (0x00000011): BPF_LOAD_ACQ
> 
> Similarly, a 16-bit BPF store-release:
> 
>   cb 21 00 00 22 00 00 00  store_release((u16 *)(r1 + 0x0), w2)
> 
>   opcode (0xcb): BPF_ATOMIC | BPF_H | BPF_STX
>   imm (0x00000022): BPF_STORE_REL
> 
> [1] https://lore.kernel.org/all/20240729183246.4110549-1-yepeilin@google.com/
> 
> Signed-off-by: Peilin Ye <yepeilin@google.com>
> ---

I think bpf_jit_supports_insn() in arch/{x86,s390}/net/bpf_jit_comp.c
need an update, as both would accept BPF_LOAD_ACQ/BPF_STORE_REL at the
moment.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> +static int check_atomic_load(struct bpf_verifier_env *env, int insn_idx,
> +			     struct bpf_insn *insn)
> +{
> +	struct bpf_reg_state *regs = cur_regs(env);
> +	int err;
> +
> +	err = check_reg_arg(env, insn->src_reg, SRC_OP);
> +	if (err)
> +		return err;
> +
> +	err = check_reg_arg(env, insn->dst_reg, DST_OP_NO_MARK);
> +	if (err)
> +		return err;
> +
> +	if (!atomic_ptr_type_ok(env, insn->src_reg, insn)) {
> +		verbose(env, "BPF_ATOMIC loads from R%d %s is not allowed\n",
> +			insn->src_reg,
> +			reg_type_str(env, reg_state(env, insn->src_reg)->type));
> +		return -EACCES;
> +	}
> +
> +	if (is_arena_reg(env, insn->src_reg)) {
> +		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
> +		if (err)
> +			return err;

Nit: this and the next function look very similar to processing of
     generic load and store in do_check(). Maybe extract that code
     as an auxiliary function and call it in both places?
     The only major difference is is_arena_reg() check guarding
     save_aux_ptr_type(), but I think it is ok to do save_aux_ptr_type
     unconditionally. Fwiw, the code would be a bit simpler,
     just spent half an hour convincing myself that such conditional handling
     is not an error. Wdyt?

> +	}
> +
> +	/* Check whether we can read the memory. */
> +	err = check_mem_access(env, insn_idx, insn->src_reg, insn->off,
> +			       BPF_SIZE(insn->code), BPF_READ, insn->dst_reg,
> +			       true, false);
> +	if (err)
> +		return err;
> +
> +	err = reg_bounds_sanity_check(env, &regs[insn->dst_reg], "atomic_load");
> +	if (err)
> +		return err;
> +	return 0;
> +}
> +
> +static int check_atomic_store(struct bpf_verifier_env *env, int insn_idx,
> +			      struct bpf_insn *insn)
> +{
> +	int err;
> +
> +	err = check_reg_arg(env, insn->src_reg, SRC_OP);
> +	if (err)
> +		return err;
> +
> +	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> +	if (err)
> +		return err;
> +
> +	if (is_pointer_value(env, insn->src_reg)) {
> +		verbose(env, "R%d leaks addr into mem\n", insn->src_reg);
> +		return -EACCES;
> +	}
> +
> +	if (!atomic_ptr_type_ok(env, insn->dst_reg, insn)) {
> +		verbose(env, "BPF_ATOMIC stores into R%d %s is not allowed\n",
> +			insn->dst_reg,
> +			reg_type_str(env, reg_state(env, insn->dst_reg)->type));
> +		return -EACCES;
> +	}
> +
> +	if (is_arena_reg(env, insn->dst_reg)) {
> +		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
> +		if (err)
> +			return err;
> +	}
> +
> +	/* Check whether we can write into the memory. */
> +	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> +			       BPF_SIZE(insn->code), BPF_WRITE, insn->src_reg,
> +			       true, false);
> +	if (err)
> +		return err;
> +	return 0;
> +}
> +

[...]