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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Deferred time check causes massive timeout multiplication on architectures with precise waits.
- [High] Unchecked negative initial timeout leads to an infinite hardware sleep.
- [High] Generic local variable names in the macro cause dangerous variable shadowing.
- [Medium] Missing parentheses around the `timeout_ns` macro argument.
--

--- Patch [1]: [PATCH v14 01/15] asm-generic: barrier: Add smp_cond_load_relaxed_timeout() ---
commit fd56d26a441207368c7c3704e10f44093698b198
Author: Ankur Arora <ankur.a.arora@oracle.com>
Subject: asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

This patch introduces smp_cond_load_relaxed_timeout, extending the relaxed
condition loader with a timeout mechanism. It implements a loop with
cpu_poll_relax and defers time checks to a slowpath to minimize performance
overhead.

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index d4f581c1e21da..ecdaddbbd7b64 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
[ ... ]
> +#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;		\

[Severity: High]
Could these generic variable names cause variable shadowing issues?

If a caller passes an expression for time_expr_ns or cond_expr that uses a
variable named __count or __timeout, would it evaluate using the macro's
internal variables instead of the caller's? For example, if the caller's
timeout variable is named __timeout, might it initialize the macro's local
variable with its own uninitialized value?

> +	s64 __timeout = (s64)timeout_ns;				\

[Severity: Medium]
Is it possible for operator precedence to cause incorrect timeout calculations
here?

Since the timeout_ns macro argument is not enclosed in parentheses during the
cast, if a caller passes an expression like a << b, the cast would apply only
to the first operand instead of the entire expression.

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

[Severity: High]
What happens if the initial timeout_ns passed by the caller is zero or
negative?

Because the time validation is deferred for SMP_TIMEOUT_POLL_COUNT iterations,
a negative initial timeout is cast to u64 on the very first loop iteration. On
architectures that implement a precise hardware wait, could this extremely
large unsigned value cause an infinite hardware sleep instead of returning
immediately?

> +		if (++__count < __spin)					\
> +			continue;					\

[Severity: High]
Does this loop inadvertently multiply the wait time on architectures with
precise waits?

Since __timeout is repeatedly passed to cpu_poll_relax without being
decremented inside the SMP_TIMEOUT_POLL_COUNT deferral loop, if the CPU wakes
up spuriously, it appears it will sleep again for the full initial timeout
duration up to 200 times. Could this cause significant latency spikes?

[ ... ]

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