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 09:55:51 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Chris Wilson <chris@...is-wilson.co.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	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 <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] x86: Add an explicit barrier() to clflushopt()

On Thu, Jan 7, 2016 at 2:16 AM, Chris Wilson <chris@...is-wilson.co.uk> wrote:
> 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.

Unless I'm mis-reading the asm, clflush_cache_range() is compiled
correctly for me.  (I don't know what the %P is for in the asm, but
that shouldn't matter.)  The ALTERNATIVE shouldn't even be visible to
the optimizer.

Can you attach a bad .s file and let us know what gcc version this is?
 (You can usually do 'make foo/bar/baz.s' to get a .s file.)  I'd also
be curious whether changing clflushopt to clwb works around the issue.

--Andy