Re: [PATCH bpf-next v2 1/6] bpf: Derive the atomic load register in one place

[Jakub Sitnicki <jakub@cloudflare.com>]

On Tue, Aug 11, 2026 at 12:18 AM +02, Daniel Borkmann wrote:
> check_atomic_rmw() open codes the mapping from a BPF_ATOMIC to the register
> it reads the old value into, the BPF_STX case of insn_def_regno() open codes
> the very same mapping a second time, and BPF JITs need it as well to know
> which register a faulting BPF_PROBE_ATOMIC has to clear. Having the
> derivations sit in different files is how the JITs came to disagree with
> the verifier in the first place. Add a small helper so that all of them can
> share it. No functional change. The BPF_LOAD_ACQ case is there for the JITs,
> which do walk all instruction classes.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  v1 -> v2:
>    - also convert insn_def_regno (Eduard, sashiko)
>
>  include/linux/filter.h | 24 ++++++++++++++++++++++++
>  kernel/bpf/fixups.c    | 11 +----------
>  kernel/bpf/verifier.c  | 17 ++++++-----------
>  3 files changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 4edba8182db1..15d83684c6e9 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -414,6 +414,30 @@ static inline bool bpf_atomic_is_load_acq(const struct bpf_insn *insn)
>  	       insn->imm == BPF_LOAD_ACQ;
>  }
>  
> +/*
> + * Given an instruction @insn, return the number of the BPF register that a
> + * BPF_ATOMIC reads the value at its memory operand into, or -1 if there is
> + * no such register. That is the register a BPF_PROBE_ATOMIC has to clear when
> + * the access faults. Like bpf_atomic_is_load_acq(), @insn is not assumed to
> + * be a BPF_ATOMIC here.
> + */
> +static inline int bpf_atomic_load_reg(const struct bpf_insn *insn)
> +{
> +	if (BPF_CLASS(insn->code) != BPF_STX ||
> +	    (BPF_MODE(insn->code) != BPF_ATOMIC &&
> +	     BPF_MODE(insn->code) != BPF_PROBE_ATOMIC))
> +		return -1;
> +
> +	switch (insn->imm) {
> +	case BPF_LOAD_ACQ:
> +		return insn->dst_reg;
> +	case BPF_CMPXCHG:
> +		return BPF_REG_0;
> +	default:
> +		return (insn->imm & BPF_FETCH) ? insn->src_reg : -1;
> +	}
> +}
> +
>  /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
>  
>  #define BPF_ST_MEM(SIZE, DST, OFF, IMM)				\
> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 661e2d13a604..c4bd70befbb5 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -49,16 +49,7 @@ static int insn_def_regno(const struct bpf_insn *insn)
>  	case BPF_ST:
>  		return -1;
>  	case BPF_STX:
> -		if (BPF_MODE(insn->code) == BPF_ATOMIC ||
> -		    BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
> -			if (insn->imm == BPF_CMPXCHG)
> -				return BPF_REG_0;
> -			else if (insn->imm == BPF_LOAD_ACQ)
> -				return insn->dst_reg;
> -			else if (insn->imm & BPF_FETCH)
> -				return insn->src_reg;
> -		}
> -		return -1;
> +		return bpf_atomic_load_reg(insn);
>  	default:
>  		return insn->dst_reg;
>  	}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index add3affc5703..73a2e8bb1782 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6485,21 +6485,16 @@ static int check_atomic_rmw(struct bpf_verifier_env *env,
>  		return -EACCES;
>  	}
>  
> -	if (insn->imm & BPF_FETCH) {
> -		if (insn->imm == BPF_CMPXCHG)
> -			load_reg = BPF_REG_0;
> -		else
> -			load_reg = insn->src_reg;
> -
> +	/*
> +	 * A negative load_reg means that this instruction accesses a memory
> +	 * location but doesn't actually load it into a register.
> +	 */

Nit: This sounds like it's part of bpf_atomic_load_reg doc. Describes
the return value. Consider moving.

> +	load_reg = bpf_atomic_load_reg(insn);
> +	if (load_reg >= 0) {
>  		/* check and record load of old value */
>  		err = check_reg_arg(env, load_reg, DST_OP);
>  		if (err)
>  			return err;
> -	} else {
> -		/* This instruction accesses a memory location but doesn't
> -		 * actually load it into a register.
> -		 */
> -		load_reg = -1;
>  	}
>  
>  	dst_reg = cur_regs(env) + insn->dst_reg;