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

On 8/11/26 1:38 AM, [email protected] wrote:
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index 4edba8182db1b..15d83684c6e94 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 661e2d13a6042..c4bd70befbb5c 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 add3affc57035..73a2e8bb1782a 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.
>> +	 */
>> +	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;
> 
> This isn't a bug, but since the point of bpf_atomic_load_reg() is to keep
> this derivation in one place, would it be worth converting the remaining
> instances in const_fold.c and liveness.c as well?
> 
> Both files have the same three-way BPF_CMPXCHG/BPF_LOAD_ACQ/BPF_FETCH
> mapping behind a BPF_STX + BPF_MODE == BPF_ATOMIC guard. In const_fold.c
> around line 204-209 it marks the register as unknown, and in liveness.c
> around line 1212-1217 it marks the register as none. Each could read as
> 'int r = bpf_atomic_load_reg(insn); if (r >= 0) <clear r>;' instead of
> repeating the switch.
Ack, I have it locally in a v3 of the series.