Re: [PATCH bpf v3] powerpc/bpf: enforce full ordering for ATOMIC operations with BPF_FETCH

Puranjay Mohan
2024-05-13T16:00:02Z

Naveen N Rao <naveen@kernel.org> writes:

> On Mon, May 13, 2024 at 10:02:48AM GMT, Puranjay Mohan wrote:
>> The Linux Kernel Memory Model [1][2] requires RMW operations that have a
>> return value to be fully ordered.
>>
>> BPF atomic operations with BPF_FETCH (including BPF_XCHG and
>> BPF_CMPXCHG) return a value back so they need to be JITed to fully
>> ordered operations. POWERPC currently emits relaxed operations for
>> these.
>>
>> We can show this by running the following litmus-test:
>>
>> PPC SB+atomic_add+fetch
>>
>> {
>> 0:r0=x; (* dst reg assuming offset is 0 *)
>> 0:r1=2; (* src reg *)
>> 0:r2=1;
>> 0:r4=y; (* P0 writes to this, P1 reads this *)
>> 0:r5=z; (* P1 writes to this, P0 reads this *)
>> 0:r6=0;
>>
>> 1:r2=1;
>> 1:r4=y;
>> 1:r5=z;
>> }
>>
>> P0 | P1 ;
>> stw r2, 0(r4) | stw r2,0(r5) ;
>> | ;
>> loop:lwarx r3, r6, r0 | ;
>> mr r8, r3 | ;
>> add r3, r3, r1 | sync ;
>> stwcx. r3, r6, r0 | ;
>> bne loop | ;
>> mr r1, r8 | ;
>> | ;
>> lwa r7, 0(r5) | lwa r7,0(r4) ;
>>
>> ~exists(0:r7=0 /\ 1:r7=0)
>>
>> Witnesses
>> Positive: 9 Negative: 3
>> Condition ~exists (0:r7=0 /\ 1:r7=0)
>> Observation SB+atomic_add+fetch Sometimes 3 9
>>
>> This test shows that the older store in P0 is reordered with a newer
>> load to a different address. Although there is a RMW operation with
>> fetch between them. Adding a sync before and after RMW fixes the issue:
>>
>> Witnesses
>> Positive: 9 Negative: 0
>> Condition ~exists (0:r7=0 /\ 1:r7=0)
>> Observation SB+atomic_add+fetch Never 0 9
>>
>> [1] https://www.kernel.org/doc/Documentation/memory-barriers.txt
>> [2] https://www.kernel.org/doc/Documentation/atomic_t.txt
>>
>> Fixes: 65112709115f ("powerpc/bpf/64: add support for BPF_ATOMIC bitwise operations")
>
> As I noted in v2, I think that is the wrong commit. This fixes the below

Sorry for missing this. Would this need another version or your message
below will make it work with the stable process?

> four commits in mainline:
> Fixes: aea7ef8a82c0 ("powerpc/bpf/32: add support for BPF_ATOMIC bitwise operations")
> Fixes: 2d9206b22743 ("powerpc/bpf/32: Add instructions for atomic_[cmp]xchg")
> Fixes: dbe6e2456fb0 ("powerpc/bpf/64: add support for atomic fetch operations")
> Fixes: 1e82dfaa7819 ("powerpc/bpf/64: Add instructions for atomic_[cmp]xchg")
>
> Cc: stable@vger.kernel.org # v6.0+

Thanks,
Puranjay