Re: [PATCH bpf-next 4/6] bpf, arm64: Clear fetch destination on faulting arena atomic

[Puranjay Mohan <puranjay@kernel.org>]

Eduard Zingerman <eddyz87@gmail.com> writes:

> On Mon, 2026-08-10 at 15:43 +0200, Daniel Borkmann wrote:
>> Same problem as on x86-64: add_exception_handler() folds "there is no
>> destination register to clear" and "this is a store" into one DONT_CLEAR
>> value ...
>> 
>>   if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn))
>>           dst_reg = DONT_CLEAR;
>> 
>> ... which ex_handler_bpf() then reads back as the access direction:
>> 
>>   bool is_write = (dst_reg == DONT_CLEAR);
>> 
>> A RMW carrying BPF_FETCH is both. emit_lse_atomic() reads the old value
>> into src_reg for BPF_{ADD,AND,OR,XOR} | BPF_FETCH and BPF_XCHG, and into
>> r0 for BPF_CMPXCHG, so a fault over an unmapped arena page is correctly
>> reported as a WRITE but leaves that register holding a stale value instead
>> of the 0 that every other BPF_PROBE_* access delivers. Same as on x86-64,
>> add a separate ARENA_WRITE bit for the direction and fill FIXUP_REG in
>> from bpf_atomic_load_reg().
>> 
>> Fixes: e612b5c1d3ee ("bpf, arm64: Add support for lse atomics in bpf_arena")
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Puranjay Mohan <puranjay@kernel.org>
>> ---
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
>>  arch/arm64/net/bpf_jit_comp.c | 36 +++++++++++++++++++++++++----------
>>  1 file changed, 26 insertions(+), 10 deletions(-)
>> 
>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>> index d14d297ebb96..796ff9193cfb 100644
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
>
> ...
>
>> @@ -1183,13 +1187,25 @@ static int add_exception_handler(const struct bpf_insn *insn,
>>  	 * dst_reg like a BPF_LDX does, hence it must not be treated as a store
>>  	 * here.
>>  	 */
>> -	if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn))
>> -		dst_reg = DONT_CLEAR;
>> +	if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn)) {
>> +		/*
>> +		 * A store has no destination register to clear, except for a
>> +		 * read-modify-write with BPF_FETCH, which also reads the old
>> +		 * value into src_reg, or into r0 for a BPF_CMPXCHG. Either way
>> +		 * the access is still reported as a write.
>> +		 */
>> +		int load_reg = bpf_atomic_load_reg(insn);
>
> Nit: I think it would be more in line with the current arm64 jit organization
>      if bpf_atomic_load_reg() call is moved to the add_exception_handler()
>      callsite in the build_insn(), where it handles BPF_PROBE_ATOMIC.
>

Wouldn't that cause more code churn?