Re: [PATCH v12 07/15] atomic: Add atomic_cond_read_*_timeout()

[Ankur Arora <ankur.a.arora@oracle.com>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The `atomic64_cond_read_acquire_timeout` and `atomic64_cond_read_relaxed_timeout` macros can cause torn reads and synchronization failures on 32-bit architectures.
> --
>
> commit aafb3af0335d368d82aaca86bf6b2a18f8ae6033
> Author: Ankur Arora <ankur.a.arora@oracle.com>
>
> atomic: Add atomic_cond_read_*_timeout()
>
> Add atomic load wrappers, atomic_cond_read_*_timeout() and
> atomic64_cond_read_*_timeout() for the cond-load timeout interfaces. Also
> add a short description for the interfaces.
>
>> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
>> index 8dd57c3a99e9b..5bcb86e077845 100644
>> --- a/include/linux/atomic.h
>> +++ b/include/linux/atomic.h
>
> [ ... ]
>
>> @@ -31,6 +31,16 @@
>>  #define atomic64_cond_read_acquire(v, c) smp_cond_load_acquire(&(v)->counter, (c))
>>  #define atomic64_cond_read_relaxed(v, c) smp_cond_load_relaxed(&(v)->counter, (c))
>>
>> +#define atomic_cond_read_acquire_timeout(v, c, e, t) \
>> +	smp_cond_load_acquire_timeout(&(v)->counter, (c), (e), (t))
>> +#define atomic_cond_read_relaxed_timeout(v, c, e, t) \
>> +	smp_cond_load_relaxed_timeout(&(v)->counter, (c), (e), (t))
>> +
>> +#define atomic64_cond_read_acquire_timeout(v, c, e, t) \
>> +	smp_cond_load_acquire_timeout(&(v)->counter, (c), (e), (t))
>> +#define atomic64_cond_read_relaxed_timeout(v, c, e, t) \
>> +	smp_cond_load_relaxed_timeout(&(v)->counter, (c), (e), (t))
>
> [Severity: High]
> Will this cause torn reads and synchronization failures on 32-bit
> architectures?
>
> On 32-bit platforms, atomic64_t operations typically rely on spinlocks or
> specific instructions to prevent tearing. These new macros bypass the
> architecture-specific atomic64_read() and directly use
> smp_cond_load_acquire_timeout() and smp_cond_load_relaxed_timeout() on the
> counter member.
>
> These macros poll using a lockless READ_ONCE(). On 32-bit systems, a 64-bit
> READ_ONCE() compiles into two separate 32-bit memory loads. If concurrent
> writers modify the counter via spinlocked atomic64 operations, the lockless
> polling loop could observe torn (partially updated) reads.


So, yes 32bit would run into torn reads but it shouldn't really be using
atomic64_cond_* interfaces.

Those interfaces are typically used via atomic_long_cond -- which should
be using atomic64_cond_* only if CONFIG_64BIT is defined.

Ankur

> If a torn read spuriously satisfies the polling condition, the loop might exit
> prematurely with a corrupt value.