Re: [PATCH v13 06/15] asm-generic: barrier: Add smp_cond_load_acquire_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `smp_cond_load_acquire_timeout` fails to provide LOAD->STORE ordering on the timeout path, violating its documented ACQUIRE guarantees.
- [Medium] The macro `smp_cond_load_acquire_timeout` evaluates its `ptr` argument after declaring a local variable named `VAL`. If the caller's `ptr` argument contains the identifier `VAL`, it will bind to the uninitialized local variable instead of the caller's variable.
--

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

asm-generic: barrier: Add smp_cond_load_acquire_timeout()

This commit adds the acquire variant of smp_cond_load_relaxed_timeout().
It reuses the relaxed variant and adds LOAD->LOAD ordering via
smp_acquire__after_ctrl_dep() to ensure ordering on the timeout path.

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

[ ... ]

> @@ -352,6 +352,46 @@ do {									\
>  })
>  #endif
>  
> +/**
> + * smp_cond_load_acquire_timeout() - (Spin) wait for cond until a timeout
> + * expires. ACQUIRE ordering when @cond_expr is satisfied.
> + * @ptr: pointer to the variable to wait on.
> + * @cond_expr: boolean expression to wait for.
> + * @time_expr_ns: monotonic expression that evaluates to time in ns or,
> + *  on failure, returns a negative value.
> + * @timeout_ns: timeout value in ns
> + * (Both of the above are assumed to be compatible with s64.)
> + *
> + * Equivalent to using smp_cond_load_acquire() on the condition variable with
> + * a timeout.
> + */
> +#ifndef smp_cond_load_acquire_timeout
> +#define smp_cond_load_acquire_timeout(ptr, cond_expr,			\
> +				      time_expr_ns, timeout_ns)		\
> +({									\
> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
> +	VAL = smp_cond_load_relaxed_timeout(ptr, cond_expr,		\
> +					     time_expr_ns,		\
> +					     timeout_ns);		\

[Severity: Medium]
Could this local VAL declaration cause macro shadowing if a caller passes an
expression for ptr that contains VAL (such as &locks[VAL])?

If this occurs, it might bind to the uninitialized local VAL variable instead
of the caller's variable, causing the loop to poll on an incorrect memory
address.

> +	/*								\
> +	 * We arrive here once the loop condition is hit, on timeout,	\
> +	 * or, if we hit both the timeout and the loop condition.	\
> +	 *								\
> +	 * The last case is low probability, but possible in the last	\
> +	 * iteration, especially on architectures with waiting		\
> +	 * cpu_poll_relax() implementations (ex. arm64).		\
> +	 * Now since the loop condition is not evaluated on timeout,	\
> +	 * we have a missed control dependency.				\
> +	 *								\
> +	 * So, force a re-evaluation of the control dependency to	\
> +	 * provide an ACQUIRE ordering for that case as well.		\
> +	 */								\
> +	if (cond_expr)							\
> +		smp_acquire__after_ctrl_dep();				\
> +	(typeof(*(ptr)))VAL;						\
> +})

[Severity: High]
Since control dependencies only order stores that are executed within the
body of the if-statement according to the memory model, is LOAD->STORE
ordering missing on the timeout path?

The caller's subsequent code executes outside of this if (cond_expr) block,
so could this allow critical section stores to be hoisted above the lock
acquisition read?

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