Re: [PATCH v12 01/15] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Valid timestamp of exactly 0 is incorrectly treated as a clock failure.
- [Medium] Negative timeout causes unintended long hardware sleep due to u64 cast.
- [Low] Macro variable shadowing due to generic local variable names.
--

commit 7f28757f0549fdb8beb7da7fa09e815a4f266f53
Author: Ankur Arora <ankur.a.arora@oracle.com>

asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

This commit adds smp_cond_load_relaxed_timeout(), extending the relaxed
condition load to allow waiting for a condition or until a timeout expires.

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index d4f581c1e21da..c56df9513a084 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h

[ ... ]

> +/*
> + * Platforms with ARCH_HAS_CPU_RELAX have a cpu_poll_relax() implementation
> + * that is expected to be cheaper (lower power) than pure polling.
> + */
> +#ifndef cpu_poll_relax
> +#define cpu_poll_relax(ptr, val, timeout_ns)	cpu_relax()
> +#endif
> +
> +/**
> + * smp_cond_load_relaxed_timeout() - (Spin) wait for cond with no ordering
> + * guarantees until a timeout expires.

[ ... ]

> +#ifndef smp_cond_load_relaxed_timeout
> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr,			\
> +				      time_expr_ns, timeout_ns)		\
> +({									\
> +	typeof(ptr) __PTR = (ptr);					\
> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
> +	u32 __count = 0, __spin = SMP_TIMEOUT_POLL_COUNT;		\
> +	s64 __timeout = (s64)timeout_ns;				\

[Severity: Low]
Does this code risk silent macro variable shadowing? The generic names like
__timeout, __count, __time_now, and __time_end do not have unique prefixes.

If a caller passes a variable named __timeout as the timeout_ns argument,
this expands to s64 __timeout = (s64)__timeout, silently masking the caller's
variable and initializing the internal variable with uninitialized garbage.

> +	s64 __time_now, __time_end = 0;					\
> +									\
> +	for (;;) {							\
> +		VAL = READ_ONCE(*__PTR);				\
> +		if (cond_expr)						\
> +			break;						\
> +		cpu_poll_relax(__PTR, VAL, (u64)__timeout);		\

[Severity: Medium]
Will this result in an unintended long hardware sleep if a caller provides a
negative timeout_ns value?

If a negative value (such as an already expired deadline) is stored in the
signed __timeout variable, casting it to u64 results in a massive unsigned
value near U64_MAX. On architectures where cpu_poll_relax() uses this for
cycle counts, this might cause the CPU to sleep indefinitely instead of
failing fast.

> +		if (++__count < __spin)					\
> +			continue;					\
> +		__time_now = (s64)(time_expr_ns);			\
> +		if (unlikely(__time_end == 0))				\
> +			__time_end = __time_now + __timeout;		\
> +		__timeout = __time_end - __time_now;			\
> +		if (__time_now <= 0 || __timeout <= 0) {		\

[Severity: High]
Does this code mistakenly treat a valid timestamp of exactly 0 as a clock
failure?

During very early boot, a valid monotonic clock like local_clock() can return
exactly 0. If this occurs, the wait spuriously aborts on its first time check,
which could cause callers to falsely assume the lock condition was met and
lead to lock queue corruption.

> +			VAL = READ_ONCE(*__PTR);			\
> +			break;						\
> +		}							\
> +		__count = 0;						\
> +	}								\
> +	(typeof(*(ptr)))VAL;						\
> +})
> +#endif

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