linux-kernel - Re: x86/idle: Remove barriers for X86_BUG_CLFLUSH

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: <20250402095306.GBZ-0JAkfivL09kG1F@fat_crate.local>
Date: Wed, 2 Apr 2025 11:53:06 +0200
From: Borislav Petkov <bp@...en8.de>
To: Andrew Cooper <andrew.cooper3@...rix.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: x86/idle: Remove barriers for X86_BUG_CLFLUSH_MONITOR

On Wed, Apr 02, 2025 at 10:10:17AM +0100, Andrew Cooper wrote:
> Commit 7e98b7192046 ("x86, idle: Use static_cpu_has() for CLFLUSH
> workaround, add barriers") adds barriers, justified with:
> 
>   ... and add memory barriers around it since the documentation is explicit
>   that CLFLUSH is only ordered with respect to MFENCE.
> 
> The SDM currently states:
> 
>   Executions of the CLFLUSH instruction are ordered with respect to each
>   other and with respect to writes, locked read-modify-write instructions,
>   and fence instructions[1].
> 
> With footnote 1 reading:
> 
>   Earlier versions of this manual specified that executions of the CLFLUSH
>   instruction were ordered only by the MFENCE instruction.  All processors
>   implementing the CLFLUSH instruction also order it relative to the other
>   operations enumerated above.
> 
> i.e. The SDM was incorrect at the time, and barriers should not have been
> inserted.  Double checking the original AAI65 errata (not available from
> intel.com any more) shows no mention of barriers either.
> 
> Additionally, drop the static_cpu_has_bug() and use a plain alternative.
> The workaround is a single instruction, with identical address setup to the
> MONITOR instruction.
> 
> Link: https://web.archive.org/web/20090219054841/http://download.intel.com/design/xeon/specupdt/32033601.pdf
> Fixes: 7e98b7192046 ("x86, idle: Use static_cpu_has() for CLFLUSH workaround, add barriers")
> Signed-off-by: Andrew Cooper <andrew.cooper3@...rix.com>
> ---
> CC: Thomas Gleixner <tglx@...utronix.de>
> CC: Ingo Molnar <mingo@...hat.com>
> CC: Borislav Petkov <bp@...en8.de>
> CC: Dave Hansen <dave.hansen@...ux.intel.com>
> CC: x86@...nel.org
> CC: "H. Peter Anvin" <hpa@...or.com>
> CC: linux-kernel@...r.kernel.org
> 
> diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> index ce857ef54cf1..dff9e7d854ed 100644
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -116,13 +116,11 @@ static __always_inline void __sti_mwait(unsigned long eax, unsigned long ecx)
>  static __always_inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
>  {
>  	if (static_cpu_has_bug(X86_BUG_MONITOR) || !current_set_polling_and_test()) {
> -		if (static_cpu_has_bug(X86_BUG_CLFLUSH_MONITOR)) {
> -			mb();
> -			clflush((void *)&current_thread_info()->flags);
> -			mb();
> -		}
> +		const void *addr = &current_thread_info()->flags;
>  
> -		__monitor((void *)&current_thread_info()->flags, 0, 0);
> +		alternative_input("", "clflush (%[addr])", X86_BUG_CLFLUSH_MONITOR,
> +				  [addr] "a" (addr));
> +		__monitor(addr, 0, 0);
>  
>  		if (!need_resched()) {
>  			if (ecx & 1) {

LGTM.

Acked-by: Borislav Petkov (AMD) <bp@...en8.de>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette