linux-kernel - Re: [PATCH 3/4] iopoll: Reorder the timeout handling in poll_timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <00600fbcc113777ef43b47f41e9c5f46aa701a83@intel.com>
Date: Thu, 03 Jul 2025 15:00:07 +0300
From: Jani Nikula <jani.nikula@...el.com>
To: Ville Syrjala <ville.syrjala@...ux.intel.com>, linux-kernel@...r.kernel.org
Cc: Lucas De Marchi <lucas.demarchi@...el.com>, Dibin Moolakadan
 Subrahmanian <dibin.moolakadan.subrahmanian@...el.com>, Imre Deak
 <imre.deak@...el.com>, David Laight <david.laight.linux@...il.com>, Geert
 Uytterhoeven <geert+renesas@...der.be>, Matt Wagantall
 <mattw@...eaurora.org>, Dejin Zheng <zhengdejin5@...il.com>,
 intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org, Ville
 Syrjälä <ville.syrjala@...ux.intel.com>
Subject: Re: [PATCH 3/4] iopoll: Reorder the timeout handling in
 poll_timeout_us()

On Thu, 03 Jul 2025, Ville Syrjala <ville.syrjala@...ux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@...ux.intel.com>
>
> Currently poll_timeout_us() evaluates 'op' and 'cond' twice
> within the loop, once at the start, and a second time after
> the timeout check. While it's probably not a big deal to do
> it twice almost back to back, it does make the macro a bit messy.
>
> Simplify the implementation to evaluate the timeout at the
> very start, then follow up with 'op'/'cond', and finally
> check if the timeout did in fact happen or not.
>
> For good measure throw in a compiler barrier between the timeout
> and 'op'/'cond' evaluations to make sure the compiler can't reoder
> the operations (which could cause false positive timeouts).
> The similar i915 __wait_for() macro already has the barrier, though
> there it is between the 'op' and 'cond' evaluations, which seems
> like it could still allow 'op' and the timeout evaluations to get
> reordered incorrectly. I suppose the ktime_get() might itself act
> as a sufficient barrier here, but better safe than sorry I guess.
>
> Cc: Jani Nikula <jani.nikula@...el.com>
> Cc: Lucas De Marchi <lucas.demarchi@...el.com>
> Cc: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@...el.com>
> Cc: Imre Deak <imre.deak@...el.com>
> Cc: David Laight <david.laight.linux@...il.com>
> Cc: Geert Uytterhoeven <geert+renesas@...der.be>
> Cc: Matt Wagantall <mattw@...eaurora.org>
> Cc: Dejin Zheng <zhengdejin5@...il.com>
> Cc: intel-gfx@...ts.freedesktop.org
> Cc: intel-xe@...ts.freedesktop.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@...ux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@...el.com>

> ---
>  include/linux/iopoll.h | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> index 69296e6adbf3..0e0940a60fdb 100644
> --- a/include/linux/iopoll.h
> +++ b/include/linux/iopoll.h
> @@ -41,18 +41,17 @@
>  	if ((sleep_before_op) && __sleep_us) \
>  		usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
>  	for (;;) { \
> +		bool __expired = __timeout_us && \
> +			ktime_compare(ktime_get(), __timeout) > 0; \
> +		/* guarantee 'op' and 'cond' are evaluated after timeout expired */ \
> +		barrier(); \
>  		op; \
>  		if (cond) { \
>  			___ret = 0; \
>  			break; \
>  		} \
> -		if (__timeout_us && \
> -		    ktime_compare(ktime_get(), __timeout) > 0) { \
> -			op; \
> -			if (cond) \
> -				___ret = 0; \
> -			else \
> -				___ret = -ETIMEDOUT; \
> +		if (__expired) { \
> +			___ret = -ETIMEDOUT; \
>  			break; \
>  		} \
>  		if (__sleep_us) \
> @@ -97,17 +96,16 @@
>  			__left_ns -= __delay_ns; \
>  	} \
>  	for (;;) { \
> +		bool __expired = __timeout_us && __left_ns < 0; \
> +		/* guarantee 'op' and 'cond' are evaluated after timeout expired */ \
> +		barrier(); \
>  		op; \
>  		if (cond) { \
>  			___ret = 0; \
>  			break; \
>  		} \
> -		if (__timeout_us && __left_ns < 0) { \
> -			op; \
> -			if (cond) \
> -				___ret = 0; \
> -			else \
> -				___ret = -ETIMEDOUT; \
> +		if (__expired) { \
> +			___ret = -ETIMEDOUT; \
>  			break; \
>  		} \
>  		if (__delay_us) { \

-- 
Jani Nikula, Intel