linux-kernel - Re: [PATCH] x86: Add an explicit barrier() to clflushopt()

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]

Date:	Thu, 7 Jan 2016 10:16:52 +0000
From:	Chris Wilson <chris@...is-wilson.co.uk>
To:	linux-kernel@...r.kernel.org
Cc:	Ross Zwisler <ross.zwisler@...ux.intel.com>,
	"H . Peter Anvin" <hpa@...ux.intel.com>,
	Andy Lutomirski <luto@...capital.net>,
	Borislav Petkov <bp@...en8.de>,
	Brian Gerst <brgerst@...il.com>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Imre Deak <imre.deak@...el.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] x86: Add an explicit barrier() to clflushopt()

On Mon, Oct 19, 2015 at 10:58:55AM +0100, Chris Wilson wrote:
> During testing we observed that the last cacheline was not being flushed
> from a
> 
> 	mb()
> 	for (addr = addr & -clflush_size; addr < end; addr += clflush_size)
> 		clflushopt();
> 	mb()
> 
> loop (where the initial addr and end were not cacheline aligned).
> 
> Changing the loop from addr < end to addr <= end, or replacing the
> clflushopt() with clflush() both fixed the testcase. Hinting that GCC
> was miscompling the assembly within the loop and specifically the
> alternative within clflushopt() was confusing the loop optimizer.
> 
> Adding a barrier() into clflushopt() is enough for GCC to dtrt, but
> solving why GCC is not seeing the constraints from the alternative_io()
> would be smarter...
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92501
> Testcase: gem_tiled_partial_pwrite_pread/read
> Signed-off-by: Chris Wilson <chris@...is-wilson.co.uk>
> Cc: Ross Zwisler <ross.zwisler@...ux.intel.com>
> Cc: H. Peter Anvin <hpa@...ux.intel.com>
> Cc: Imre Deak <imre.deak@...el.com>
> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
> Cc: dri-devel@...ts.freedesktop.org
> ---
>  arch/x86/include/asm/special_insns.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 2270e41b32fd..0c7aedbf8930 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -199,6 +199,11 @@ static inline void clflushopt(volatile void *__p)
>  		       ".byte 0x66; clflush %P0",
>  		       X86_FEATURE_CLFLUSHOPT,
>  		       "+m" (*(volatile char __force *)__p));
> +	/* GCC (4.9.1 and 5.2.1 at least) appears to be very confused when
> +	 * meeting this alternative() and demonstrably miscompiles loops
> +	 * iterating over clflushopts.
> +	 */
> +	barrier();
>  }

Or an alternative:

+#define alternative_output(oldinstr, newinstr, feature, output)        \
+       asm volatile (ALTERNATIVE(oldinstr, newinstr, feature)          \
+               : output : "i" (0) : "memory")

I would really appreciate some knowledgeable folks taking a look at the
asm for clflushopt() as it still affects today's kernel and gcc.

Fwiw, I have confirmed that arch/x86/mm/pageattr.c clflush_cache_range()
is similarly affected.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/