[PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[Matthias Kaehlcke]

The constraint "rm" allows the compiler to put mix_const into memory.
When the input operand is a memory location mul needs an operand size
suffix, since it can't infer the multiplication width from the operand.

Add and use the _ASM_MUL macro which determines the operand size and
resolves to the 'mul' instruction with the corresponding suffix.

This fixes the following error when building with clang:

CC      arch/x86/lib/kaslr.o
/tmp/kaslr-dfe1ad.s: Assembler messages:
/tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given and
no register operands; can't size instruction

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- updated commit message with additional details

 arch/x86/include/asm/asm.h | 1 +
 arch/x86/lib/kaslr.c       | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 7acb51c49fec..7a9df3beb89b 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -32,6 +32,7 @@
 #define _ASM_ADD	__ASM_SIZE(add)
 #define _ASM_SUB	__ASM_SIZE(sub)
 #define _ASM_XADD	__ASM_SIZE(xadd)
+#define _ASM_MUL	__ASM_SIZE(mul)
 
 #define _ASM_AX		__ASM_REG(ax)
 #define _ASM_BX		__ASM_REG(bx)
diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
index 121f59c6ee54..0c7fe444dcdd 100644
--- a/arch/x86/lib/kaslr.c
+++ b/arch/x86/lib/kaslr.c
@@ -5,6 +5,7 @@
  * kernel starts. This file is included in the compressed kernel and
  * normally linked in the regular.
  */
+#include <asm/asm.h>
 #include <asm/kaslr.h>
 #include <asm/msr.h>
 #include <asm/archrandom.h>
@@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char *purpose)
 	}
 
 	/* Circular multiply for better bit diffusion */
-	asm("mul %3"
+	asm(_ASM_MUL "%3"
 	    : "=a" (random), "=d" (raw)
 	    : "a" (random), "rm" (mix_const));
 	random += raw;
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication

[Kees Cook]

On Mon, May 1, 2017 at 3:47 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> The constraint "rm" allows the compiler to put mix_const into memory.
> When the input operand is a memory location mul needs an operand size
> suffix, since it can't infer the multiplication width from the operand.
>
> Add and use the _ASM_MUL macro which determines the operand size and
> resolves to the 'mul' instruction with the corresponding suffix.
>
> This fixes the following error when building with clang:
>
> CC      arch/x86/lib/kaslr.o
> /tmp/kaslr-dfe1ad.s: Assembler messages:
> /tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given and
> no register operands; can't size instruction
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Acked-by: Kees Cook <keescook@chromium.org>

Thanks for the v2!

-Kees

> ---
> Changes in v2:
> - updated commit message with additional details
>
>  arch/x86/include/asm/asm.h | 1 +
>  arch/x86/lib/kaslr.c       | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
> index 7acb51c49fec..7a9df3beb89b 100644
> --- a/arch/x86/include/asm/asm.h
> +++ b/arch/x86/include/asm/asm.h
> @@ -32,6 +32,7 @@
>  #define _ASM_ADD       __ASM_SIZE(add)
>  #define _ASM_SUB       __ASM_SIZE(sub)
>  #define _ASM_XADD      __ASM_SIZE(xadd)
> +#define _ASM_MUL       __ASM_SIZE(mul)
>
>  #define _ASM_AX                __ASM_REG(ax)
>  #define _ASM_BX                __ASM_REG(bx)
> diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
> index 121f59c6ee54..0c7fe444dcdd 100644
> --- a/arch/x86/lib/kaslr.c
> +++ b/arch/x86/lib/kaslr.c
> @@ -5,6 +5,7 @@
>   * kernel starts. This file is included in the compressed kernel and
>   * normally linked in the regular.
>   */
> +#include <asm/asm.h>
>  #include <asm/kaslr.h>
>  #include <asm/msr.h>
>  #include <asm/archrandom.h>
> @@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char *purpose)
>         }
>
>         /* Circular multiply for better bit diffusion */
> -       asm("mul %3"
> +       asm(_ASM_MUL "%3"
>             : "=a" (random), "=d" (raw)
>             : "a" (random), "rm" (mix_const));
>         random += raw;
> --
> 2.13.0.rc1.294.g07d810a77f-goog
>



-- 
Kees Cook
Pixel Security

[tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[tip-bot for Matthias Kaehlcke]

Commit-ID:  121843eb02a6e2fa30aefab64bfe183c97230c75
Gitweb:     http://git.kernel.org/tip/121843eb02a6e2fa30aefab64bfe183c97230c75
Author:     Matthias Kaehlcke <mka@chromium.org>
AuthorDate: Mon, 1 May 2017 15:47:41 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 5 May 2017 08:31:05 +0200

x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

The constraint "rm" allows the compiler to put mix_const into memory.
When the input operand is a memory location then MUL needs an operand
size suffix, since Clang can't infer the multiplication width from the
operand.

Add and use the _ASM_MUL macro which determines the operand size and
resolves to the NUL instruction with the corresponding suffix.

This fixes the following error when building with clang:

  CC      arch/x86/lib/kaslr.o
  /tmp/kaslr-dfe1ad.s: Assembler messages:
  /tmp/kaslr-dfe1ad.s:182: Error: no instruction mnemonic suffix given and no register operands; can't size instruction

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Cc: Grant Grundler <grundler@chromium.org>
Cc: Greg Hackmann <ghackmann@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Davidson <md@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170501224741.133938-1-mka@chromium.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/asm.h | 1 +
 arch/x86/lib/kaslr.c       | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 7acb51c..7a9df3b 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -32,6 +32,7 @@
 #define _ASM_ADD	__ASM_SIZE(add)
 #define _ASM_SUB	__ASM_SIZE(sub)
 #define _ASM_XADD	__ASM_SIZE(xadd)
+#define _ASM_MUL	__ASM_SIZE(mul)
 
 #define _ASM_AX		__ASM_REG(ax)
 #define _ASM_BX		__ASM_REG(bx)
diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
index 5761a4f..ab2d1d7 100644
--- a/arch/x86/lib/kaslr.c
+++ b/arch/x86/lib/kaslr.c
@@ -5,6 +5,7 @@
  * kernel starts. This file is included in the compressed kernel and
  * normally linked in the regular.
  */
+#include <asm/asm.h>
 #include <asm/kaslr.h>
 #include <asm/msr.h>
 #include <asm/archrandom.h>
@@ -79,7 +80,7 @@ unsigned long kaslr_get_random_long(const char *purpose)
 	}
 
 	/* Circular multiply for better bit diffusion */
-	asm("mul %3"
+	asm(_ASM_MUL "%3"
 	    : "=a" (random), "=d" (raw)
 	    : "a" (random), "rm" (mix_const));
 	random += raw;

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Peter Zijlstra]

On Fri, May 05, 2017 at 01:11:47AM -0700, tip-bot for Matthias Kaehlcke wrote:
> Commit-ID:  121843eb02a6e2fa30aefab64bfe183c97230c75
> Gitweb:     http://git.kernel.org/tip/121843eb02a6e2fa30aefab64bfe183c97230c75
> Author:     Matthias Kaehlcke <mka@chromium.org>
> AuthorDate: Mon, 1 May 2017 15:47:41 -0700
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Fri, 5 May 2017 08:31:05 +0200
> 
> x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
> 
> The constraint "rm" allows the compiler to put mix_const into memory.
> When the input operand is a memory location then MUL needs an operand
> size suffix, since Clang can't infer the multiplication width from the
> operand.

*sigh*, this is another shining example of how LLVM is a better, faster
moving compiler?

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, May 05, 2017 at 01:11:47AM -0700, tip-bot for Matthias Kaehlcke wrote:
> > Commit-ID:  121843eb02a6e2fa30aefab64bfe183c97230c75
> > Gitweb:     http://git.kernel.org/tip/121843eb02a6e2fa30aefab64bfe183c97230c75
> > Author:     Matthias Kaehlcke <mka@chromium.org>
> > AuthorDate: Mon, 1 May 2017 15:47:41 -0700
> > Committer:  Ingo Molnar <mingo@kernel.org>
> > CommitDate: Fri, 5 May 2017 08:31:05 +0200
> > 
> > x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
> > 
> > The constraint "rm" allows the compiler to put mix_const into memory.
> > When the input operand is a memory location then MUL needs an operand
> > size suffix, since Clang can't infer the multiplication width from the
> > operand.
> 
> *sigh*, this is another shining example of how LLVM is a better, faster
> moving compiler?

Well, I don't like it - but we already have similar patterns to cover some asm 
complications so I didn't mind. Apparently Clang is very close to being able to 
build a working Linux kernel, right?

In that sense it would be unfair to expect it to not have various legacies, 
missing features and quirks - just like the kernel has dozens of GCC related 
workarounds.

Thanks,

	Ingo

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Peter Zijlstra]

On Fri, May 05, 2017 at 07:50:39PM +0200, Ingo Molnar wrote:

> > > The constraint "rm" allows the compiler to put mix_const into memory.
> > > When the input operand is a memory location then MUL needs an operand
> > > size suffix, since Clang can't infer the multiplication width from the
> > > operand.
> > 
> > *sigh*, this is another shining example of how LLVM is a better, faster
> > moving compiler?
> 
> Well, I don't like it - but we already have similar patterns to cover some asm 
> complications so I didn't mind. Apparently Clang is very close to being able to 
> build a working Linux kernel, right?
> 
> In that sense it would be unfair to expect it to not have various legacies, 
> missing features and quirks - just like the kernel has dozens of GCC related 
> workarounds.

The distinction is that GCC can actually compile the kernel, and has
been able for a long while. Therefore we actually have to deal with old
versions.

LLVM otoh has never compiled the kernel, so I don't see why we need to
care about its old versions.

The argument made last time is that LLVM is a fast moving compiler etc..
all I'm agitating against is that false claim. You're right that this
patch isn't too bad, but some of them were pretty hideous and until
they've fixed their compiler to deal with those, I don't see the point
of taking these.

Also, LLVM is still lacking some major features like asm goto and asm
flags output operands. Without them supporting those whatever kernel
they do manage to compile is just not worth the trouble.

So rather than try and fudge the kernel source to build on this inferior
compiler, I'd rather see them prove their point and shift LLVM into gear
and build a compiler you'd want to use.

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Matthias Kaehlcke]

El Fri, May 05, 2017 at 07:50:39PM +0200 Ingo Molnar ha dit:

> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Fri, May 05, 2017 at 01:11:47AM -0700, tip-bot for Matthias Kaehlcke wrote:
> > > Commit-ID:  121843eb02a6e2fa30aefab64bfe183c97230c75
> > > Gitweb:     http://git.kernel.org/tip/121843eb02a6e2fa30aefab64bfe183c97230c75
> > > Author:     Matthias Kaehlcke <mka@chromium.org>
> > > AuthorDate: Mon, 1 May 2017 15:47:41 -0700
> > > Committer:  Ingo Molnar <mingo@kernel.org>
> > > CommitDate: Fri, 5 May 2017 08:31:05 +0200
> > > 
> > > x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
> > > 
> > > The constraint "rm" allows the compiler to put mix_const into memory.
> > > When the input operand is a memory location then MUL needs an operand
> > > size suffix, since Clang can't infer the multiplication width from the
> > > operand.
> > 
> > *sigh*, this is another shining example of how LLVM is a better, faster
> > moving compiler?
> 
> Well, I don't like it - but we already have similar patterns to cover some asm 
> complications so I didn't mind. Apparently Clang is very close to being able to 
> build a working Linux kernel, right?

Indeed, I expect 4.12 (with this patch ...) to build with Clang for a
x86 defconfig (with tons of warnings). ARM64 is very close.

> In that sense it would be unfair to expect it to not have various legacies, 
> missing features and quirks - just like the kernel has dozens of GCC related 
> workarounds.

Also my understanding is that this isn't really a clang issue. In the
context of this code gcc apparently chooses to use a register for
'mix_const', for memory locations it also needs a suffix.

Actually I just tried to build this code from a single C file:

void test() {
  unsigned long raw, random;
  const unsigned long mix_const = 0x3f39e593UL;

  asm("MUL %3"
    : "=a" (random), "=d" (raw)
    : "a" (random), "rm" (mix_const));
}

gcc -c /tmp/test.c
/tmp/test.c: Assembler messages:
/tmp/test.c:6: Error: no instruction mnemonic suffix given and no
  register operands; can't size instruction

gcc version 4.9.x 20150123

Cheers

Matthias

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Linus Torvalds]

On Fri, May 5, 2017 at 11:44 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Indeed, I expect 4.12 (with this patch ...) to build with Clang for a
> x86 defconfig (with tons of warnings). ARM64 is very close.

Does it actually *work*, rather than just build?

clang used to have actual code generation bugs that made for really
subtle kernel issues but didn't matter very often in user space.

The thing that comes to mind is the crazy handling of spilling
'eflags' on x86 (saving and restoring eflags over a function call in
order to save the arithmetic flags, but in the process also undoing
the changes to IF that that function call did.

That was a horrible horrible bug - it generates slow code, but it was
actually *incorrect* code too. It's just that in user space, people
very seldom mess with the non-arithmetic flags (things like IOPL, IF,
AC, etc - they *can* be used in user space but seldom are).

Some of the discussion I saw by the clang people pooh-poohed that bug
and seemed to think it was a kernel problem.

That kind of pure incompetence from compiler people doesn't make me
get the warm and fuzzies.

Generating code that is actively _wrong_ and performs badly too - hey
that happens. But then making excuses for clearly buggy code
generation?

I did have a report saying it was fixed, so hopefully saner people prevailed.

But it left a really bad taste, and it was an example of something
that may have built fine, but generated subtle broken code (where
"subtly" here means "it might work when you don't look too closely and
are lucky, and then cause lockups in odd situations").

I'd love to be able to compile the kernel with clang, but only if the
clang people take it seriously.

            Linus

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[hpa]

On May 5, 2017 11:44:05 AM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>El Fri, May 05, 2017 at 07:50:39PM +0200 Ingo Molnar ha dit:
>
>> 
>> * Peter Zijlstra <peterz@infradead.org> wrote:
>> 
>> > On Fri, May 05, 2017 at 01:11:47AM -0700, tip-bot for Matthias
>Kaehlcke wrote:
>> > > Commit-ID:  121843eb02a6e2fa30aefab64bfe183c97230c75
>> > > Gitweb:    
>http://git.kernel.org/tip/121843eb02a6e2fa30aefab64bfe183c97230c75
>> > > Author:     Matthias Kaehlcke <mka@chromium.org>
>> > > AuthorDate: Mon, 1 May 2017 15:47:41 -0700
>> > > Committer:  Ingo Molnar <mingo@kernel.org>
>> > > CommitDate: Fri, 5 May 2017 08:31:05 +0200
>> > > 
>> > > x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work
>around Clang incompatibility
>> > > 
>> > > The constraint "rm" allows the compiler to put mix_const into
>memory.
>> > > When the input operand is a memory location then MUL needs an
>operand
>> > > size suffix, since Clang can't infer the multiplication width
>from the
>> > > operand.
>> > 
>> > *sigh*, this is another shining example of how LLVM is a better,
>faster
>> > moving compiler?
>> 
>> Well, I don't like it - but we already have similar patterns to cover
>some asm 
>> complications so I didn't mind. Apparently Clang is very close to
>being able to 
>> build a working Linux kernel, right?
>
>Indeed, I expect 4.12 (with this patch ...) to build with Clang for a
>x86 defconfig (with tons of warnings). ARM64 is very close.
>
>> In that sense it would be unfair to expect it to not have various
>legacies, 
>> missing features and quirks - just like the kernel has dozens of GCC
>related 
>> workarounds.
>
>Also my understanding is that this isn't really a clang issue. In the
>context of this code gcc apparently chooses to use a register for
>'mix_const', for memory locations it also needs a suffix.
>
>Actually I just tried to build this code from a single C file:
>
>void test() {
>  unsigned long raw, random;
>  const unsigned long mix_const = 0x3f39e593UL;
>
>  asm("MUL %3"
>    : "=a" (random), "=d" (raw)
>    : "a" (random), "rm" (mix_const));
>}
>
>gcc -c /tmp/test.c
>/tmp/test.c: Assembler messages:
>/tmp/test.c:6: Error: no instruction mnemonic suffix given and no
>  register operands; can't size instruction
>
>gcc version 4.9.x 20150123
>
>Cheers
>
>Matthias

Yes, this its a bug regardless of clang or not. It just happens to be hittin by the particular optimization choice is the current versions of gcc makes.

I asked that that be clear in the commit message, but by the time it was merged it has gotten muddled again.

However, the clang team also need to accept that they can't do arbitrary hacks in the kernel when their compiler is inadequate.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Michael Davidson]

On Fri, May 5, 2017 at 12:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, May 5, 2017 at 11:44 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
>>
>> Indeed, I expect 4.12 (with this patch ...) to build with Clang for a
>> x86 defconfig (with tons of warnings). ARM64 is very close.
>
> Does it actually *work*, rather than just build?
>

Yes, it does work.

I have been using clang to build and run various 4.x based versions of
Linux on both x86_64 and powerpc for over 6 months now.

Assuming that you have a wrapper for clang that sets a few options and
disables some warnings there really is very little else that is
needed.

powerpc currently only needs a single patch to the arch Makefile to
add a cc-option check to a gcc specific flag.

x86_64 needs the change that was under discussion here, but is still
blocked by issues with building the 16 bit boot code.
Supposedly this has now been fixed in clang, but I have not yet either
seen or tried a version of the compiler with that fix.

arm64 looks like it is also now very close.

There are a few lingering places in the kernel which use variable
length arrays in structs (eg the raid10 driver) which don't build with
clang and that is about it.

So, while I completely understand the resistance to adding arbitrary
hacks to the kernel just to support another compiler it is important
to also understand just how close things are to "just working".

md

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Matthias Kaehlcke]

El Fri, May 05, 2017 at 12:30:20PM -0700 Linus Torvalds ha dit:

> On Fri, May 5, 2017 at 11:44 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Indeed, I expect 4.12 (with this patch ...) to build with Clang for a
> > x86 defconfig (with tons of warnings). ARM64 is very close.
> 
> Does it actually *work*, rather than just build?

Unfortunately I can't make an universal affirmation here. The systems
for which I currently develop run a 4.4ish kernel with the clang
patches on top. Both x86 and ARM64 boot, the peripherals work and I
haven't encountered any specific problems yet. Automated tests to
assess or detect regressions are still pending.

Does it work on all possible hardware configurations and use cases?
Almost certainly not. However I think here is where having basic
upstream support for clang can help by allowing more people to
evaluate it on their systems without requiring a whole bunch of random
out-of-tree patches, which also makes it easier to contribute back.

> clang used to have actual code generation bugs that made for really
> subtle kernel issues but didn't matter very often in user space.
> 
> The thing that comes to mind is the crazy handling of spilling
> 'eflags' on x86 (saving and restoring eflags over a function call in
> order to save the arithmetic flags, but in the process also undoing
> the changes to IF that that function call did.
> 
> That was a horrible horrible bug - it generates slow code, but it was
> actually *incorrect* code too. It's just that in user space, people
> very seldom mess with the non-arithmetic flags (things like IOPL, IF,
> AC, etc - they *can* be used in user space but seldom are).
> 
> Some of the discussion I saw by the clang people pooh-poohed that bug
> and seemed to think it was a kernel problem.
> 
> That kind of pure incompetence from compiler people doesn't make me
> get the warm and fuzzies.
> 
> Generating code that is actively _wrong_ and performs badly too - hey
> that happens. But then making excuses for clearly buggy code
> generation?
> 
> I did have a report saying it was fixed, so hopefully saner people prevailed.
> 
> But it left a really bad taste, and it was an example of something
> that may have built fine, but generated subtle broken code (where
> "subtly" here means "it might work when you don't look too closely and
> are lucky, and then cause lockups in odd situations").
> 
> I'd love to be able to compile the kernel with clang, but only if the
> clang people take it seriously.

I only started looking into clang a few months ago, and can't really
comment on the cases you mention. My expectation is that performance
and stability issues will be taken quite seriously when clang kernel
builds are more widely used on business critical production systems.

Cheers

Matthias

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Matthias Kaehlcke]

Hi Peter,

El Fri, May 05, 2017 at 12:37:23PM -0700 hpa@zytor.com ha dit:

> On May 5, 2017 11:44:05 AM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
> >El Fri, May 05, 2017 at 07:50:39PM +0200 Ingo Molnar ha dit:
> >
> >> 
> >> * Peter Zijlstra <peterz@infradead.org> wrote:
> >> 
> >> > On Fri, May 05, 2017 at 01:11:47AM -0700, tip-bot for Matthias
> >Kaehlcke wrote:
> >> > > Commit-ID:  121843eb02a6e2fa30aefab64bfe183c97230c75
> >> > > Gitweb:    
> >http://git.kernel.org/tip/121843eb02a6e2fa30aefab64bfe183c97230c75
> >> > > Author:     Matthias Kaehlcke <mka@chromium.org>
> >> > > AuthorDate: Mon, 1 May 2017 15:47:41 -0700
> >> > > Committer:  Ingo Molnar <mingo@kernel.org>
> >> > > CommitDate: Fri, 5 May 2017 08:31:05 +0200
> >> > > 
> >> > > x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work
> >around Clang incompatibility
> >> > > 
> >> > > The constraint "rm" allows the compiler to put mix_const into
> >memory.
> >> > > When the input operand is a memory location then MUL needs an
> >operand
> >> > > size suffix, since Clang can't infer the multiplication width
> >from the
> >> > > operand.
> >> > 
> >> > *sigh*, this is another shining example of how LLVM is a better,
> >faster
> >> > moving compiler?
> >> 
> >> Well, I don't like it - but we already have similar patterns to cover
> >some asm 
> >> complications so I didn't mind. Apparently Clang is very close to
> >being able to 
> >> build a working Linux kernel, right?
> >
> >Indeed, I expect 4.12 (with this patch ...) to build with Clang for a
> >x86 defconfig (with tons of warnings). ARM64 is very close.
> >
> >> In that sense it would be unfair to expect it to not have various
> >legacies, 
> >> missing features and quirks - just like the kernel has dozens of GCC
> >related 
> >> workarounds.
> >
> >Also my understanding is that this isn't really a clang issue. In the
> >context of this code gcc apparently chooses to use a register for
> >'mix_const', for memory locations it also needs a suffix.
> >
> >Actually I just tried to build this code from a single C file:
> >
> >void test() {
> >  unsigned long raw, random;
> >  const unsigned long mix_const = 0x3f39e593UL;
> >
> >  asm("MUL %3"
> >    : "=a" (random), "=d" (raw)
> >    : "a" (random), "rm" (mix_const));
> >}
> >
> >gcc -c /tmp/test.c
> >/tmp/test.c: Assembler messages:
> >/tmp/test.c:6: Error: no instruction mnemonic suffix given and no
> >  register operands; can't size instruction
> >
> >gcc version 4.9.x 20150123
> >
> >Cheers
> >
> >Matthias
> 
> Yes, this its a bug regardless of clang or not. It just happens to be hittin by the particular optimization choice is the current versions of gcc makes.
> 
> I asked that that be clear in the commit message, but by the time it was merged it has gotten muddled again.
> 
> However, the clang team also need to accept that they can't do
> arbitrary hacks in the kernel when their compiler is inadequate.

AFAIK there isn't really such a thing as "the clang team" (anymore?),
or at least they don't invite me to their parties :)

Overall I totally agree that we should avoid hacks to support clang.
Short-comings in clang that require ugly or wide-spread hacks should
be fixed in the compiler and out-of-tree patches can be used as
workaround.

However there may be cases where minor innocuous changes are needed
to enable clang (this one could be an example if it was really a
clang issue) and I hope we can be pragmatic in these situations,
especially since these are/seem the last missing bits to support the
other big open source compiler out there.

Cheers

Matthias

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Peter Zijlstra]

On Fri, May 05, 2017 at 01:36:34PM -0700, Michael Davidson wrote:

> There are a few lingering places in the kernel which use variable
> length arrays in structs (eg the raid10 driver) which don't build with
> clang and that is about it.

So the other point I raised is lack of asm goto (and asm flags output).

Without that our static key infrastructure reverts to runtime branches
and affects performance.

> So, while I completely understand the resistance to adding arbitrary
> hacks to the kernel just to support another compiler it is important
> to also understand just how close things are to "just working".

Reading up on the LLVM thread on asm goto they appear to want to provide
an intrinsic to allow doing the patchable branch thing. That would be
fairly limiting, and the proposal I've seen doesn't even cover the two
(or rather 4) states of patchable branches we have in the kernel.

Not to mention that such an intrinsic doesn't even begin to cover all
the other (perhaps creative) uses we have got asm goto used.

But my main point is that we'd have to rewrite and maintain _two_
versions of the static key infrastructure if we were to support LLVM's
intrinsic and the GCC asm goto. That is a very undesirable place to be.

So while they'll say they support the feature, I'll say its worthless
since I'm not inclined to support their variant of it. As is, I'm not
getting the feeling the LLVM team really cares about Linux.

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Ingo Molnar]

* Matthias Kaehlcke <mka@chromium.org> wrote:

> El Fri, May 05, 2017 at 12:30:20PM -0700 Linus Torvalds ha dit:
> 
> > On Fri, May 5, 2017 at 11:44 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > Indeed, I expect 4.12 (with this patch ...) to build with Clang for a
> > > x86 defconfig (with tons of warnings). ARM64 is very close.
> > 
> > Does it actually *work*, rather than just build?
> 
> Unfortunately I can't make an universal affirmation here. The systems for which 
> I currently develop run a 4.4ish kernel with the clang patches on top. Both x86 
> and ARM64 boot, the peripherals work and I haven't encountered any specific 
> problems yet. Automated tests to assess or detect regressions are still pending.
> 
> Does it work on all possible hardware configurations and use cases? Almost 
> certainly not. However I think here is where having basic upstream support for 
> clang can help by allowing more people to evaluate it on their systems without 
> requiring a whole bunch of random out-of-tree patches, which also makes it 
> easier to contribute back.

So my impression too was that Clang was 'very close' to being usable by users to 
build a working Linux kernel in practice (on x86), and at this point the kernel 
can certainly accomodate a Clang quirk or two to bridge the chicken-and-egg 
problem of who supports whom first.

Note that I did NAK ugly Clang hacks in the recent past, so this tentative pledge 
of support (on the x86 arch side) is not unconditional. Peter is also right about 
proper asm goto compatibility probably being a preprequisite of any major distro 
going to Clang.

( I see Clang also as a way for the GCC folks to get their act together - the
  advantages of competition and all that. GCC has the better license after all,
  IMHO. )

Thanks,

	Ingo

Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[hpa]

On May 6, 2017 1:16:35 AM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
>On Fri, May 05, 2017 at 01:36:34PM -0700, Michael Davidson wrote:
>
>> There are a few lingering places in the kernel which use variable
>> length arrays in structs (eg the raid10 driver) which don't build
>with
>> clang and that is about it.
>
>So the other point I raised is lack of asm goto (and asm flags output).
>
>Without that our static key infrastructure reverts to runtime branches
>and affects performance.
>
>> So, while I completely understand the resistance to adding arbitrary
>> hacks to the kernel just to support another compiler it is important
>> to also understand just how close things are to "just working".
>
>Reading up on the LLVM thread on asm goto they appear to want to
>provide
>an intrinsic to allow doing the patchable branch thing. That would be
>fairly limiting, and the proposal I've seen doesn't even cover the two
>(or rather 4) states of patchable branches we have in the kernel.
>
>Not to mention that such an intrinsic doesn't even begin to cover all
>the other (perhaps creative) uses we have got asm goto used.
>
>But my main point is that we'd have to rewrite and maintain _two_
>versions of the static key infrastructure if we were to support LLVM's
>intrinsic and the GCC asm goto. That is a very undesirable place to be.
>
>So while they'll say they support the feature, I'll say its worthless
>since I'm not inclined to support their variant of it. As is, I'm not
>getting the feeling the LLVM team really cares about Linux.

This has been a common problem with LLVM: they say that they will provide feature compatibility with gcc, but then they say this or that gcc extension "doesn't make sense" and is something they don't want to support.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.