Re: [PATCH v16 07/16] atomic: Add atomic_cond_read_*_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] atomic: torn reads in atomic64_cond_read_*_timeout on 32-bit
- [Low] docs: incorrect categorization of atomic_t operations
--

commit 9e35fd2167c99d05e59317145e35667a8118c80e
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.

> diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt
> index bee3b1bca9a7b..0e53f6ccb5580 100644
> --- a/Documentation/atomic_t.txt
> +++ b/Documentation/atomic_t.txt
> @@ -16,6 +16,10 @@ Non-RMW ops:
>    atomic_read(), atomic_set()
>    atomic_read_acquire(), atomic_set_release()
>  
> +Non-RMW, non-atomic_t ops:
> +
> +  atomic_cond_read_{relaxed,acquire}()
> +  atomic_cond_read_{relaxed,acquire}_timeout()

[Severity: Low]
Does this section header miscategorize these operations?

These macros explicitly take an atomic_t pointer as their primary argument and
operate on atomic types. Was the intention to denote them as conditional wait
operations instead of non-atomic_t operations?

[ ... ]

> 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]
Could passing the address of the 64-bit counter directly to the unlocked
polling macro lead to torn reads on 32-bit architectures?

The generic atomic64_t implementation for 32-bit architectures (used when
lacking native 64-bit atomic instructions) updates the counter via two 32-bit
stores protected by a hashed array of spinlocks in lib/atomic64.c.

Because smp_cond_load_acquire_timeout polls the variable using an unlocked
READ_ONCE(), concurrent updates to the counter will bypass the spinlock.

On 32-bit architectures, READ_ONCE() on an 8-byte value evaluates successfully
but compiles into two independent 32-bit reads, which can result in data
corruption.

Additionally, smp_cond_load_acquire_timeout relies on smp_load_acquire() when
timeout_ns < 0. Can this cause a build failure on 32-bit architectures due to
compiletime_assert_atomic_type() preventing 8-byte acquire loads?

The pre-existing atomic64_cond_read_acquire and atomic64_cond_read_relaxed
macros share this identical architectural flaw, and it looks like the new
API macros propagate the same vulnerability.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016070837.440748-1-ankur.a.arora@oracle.com?part=7