LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
@ 2017-05-01 22:47 Matthias Kaehlcke
  2017-05-02  2:08 ` Kees Cook
  2017-05-05  8:11 ` [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility tip-bot for Matthias Kaehlcke
  0 siblings, 2 replies; 22+ messages in thread
From: Matthias Kaehlcke @ 2017-05-01 22:47 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Kees Cook
  Cc: x86, linux-kernel, Grant Grundler, Greg Hackmann,
	Michael Davidson, 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
  2017-05-01 22:47 [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Matthias Kaehlcke
@ 2017-05-02  2:08 ` Kees Cook
  2017-05-05  8:11 ` [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility tip-bot for Matthias Kaehlcke
  1 sibling, 0 replies; 22+ messages in thread
From: Kees Cook @ 2017-05-02  2:08 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Thomas Gleixner, Ingo Molnar, H . Peter Anvin, x86, LKML,
	Grant Grundler, Greg Hackmann, Michael Davidson

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-01 22:47 [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Matthias Kaehlcke
  2017-05-02  2:08 ` Kees Cook
@ 2017-05-05  8:11 ` tip-bot for Matthias Kaehlcke
  2017-05-05 10:25   ` Peter Zijlstra
  1 sibling, 1 reply; 22+ messages in thread
From: tip-bot for Matthias Kaehlcke @ 2017-05-05  8:11 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: tglx, hpa, peterz, torvalds, md, mka, mingo, keescook, ghackmann,
	grundler, linux-kernel

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;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05  8:11 ` [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility tip-bot for Matthias Kaehlcke
@ 2017-05-05 10:25   ` Peter Zijlstra
  2017-05-05 17:50     ` Ingo Molnar
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2017-05-05 10:25 UTC (permalink / raw)
  To: mka, md, tglx, hpa, torvalds, grundler, linux-kernel, mingo,
	ghackmann, keescook
  Cc: linux-tip-commits

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?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 10:25   ` Peter Zijlstra
@ 2017-05-05 17:50     ` Ingo Molnar
  2017-05-05 18:22       ` Peter Zijlstra
  2017-05-05 18:44       ` Matthias Kaehlcke
  0 siblings, 2 replies; 22+ messages in thread
From: Ingo Molnar @ 2017-05-05 17:50 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: mka, md, tglx, hpa, torvalds, grundler, linux-kernel, ghackmann,
	keescook, linux-tip-commits


* 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 17:50     ` Ingo Molnar
@ 2017-05-05 18:22       ` Peter Zijlstra
  2017-05-05 18:44       ` Matthias Kaehlcke
  1 sibling, 0 replies; 22+ messages in thread
From: Peter Zijlstra @ 2017-05-05 18:22 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: mka, md, tglx, hpa, torvalds, grundler, linux-kernel, ghackmann,
	keescook, linux-tip-commits

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 17:50     ` Ingo Molnar
  2017-05-05 18:22       ` Peter Zijlstra
@ 2017-05-05 18:44       ` Matthias Kaehlcke
  2017-05-05 19:30         ` Linus Torvalds
  2017-05-05 19:37         ` hpa
  1 sibling, 2 replies; 22+ messages in thread
From: Matthias Kaehlcke @ 2017-05-05 18:44 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Peter Zijlstra, md, tglx, hpa, torvalds, grundler, linux-kernel,
	ghackmann, keescook, linux-tip-commits

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 18:44       ` Matthias Kaehlcke
@ 2017-05-05 19:30         ` Linus Torvalds
  2017-05-05 20:36           ` Michael Davidson
  2017-05-05 20:52           ` Matthias Kaehlcke
  2017-05-05 19:37         ` hpa
  1 sibling, 2 replies; 22+ messages in thread
From: Linus Torvalds @ 2017-05-05 19:30 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Ingo Molnar, Peter Zijlstra, Michael Davidson, Thomas Gleixner,
	Peter Anvin, grundler, Linux Kernel Mailing List, ghackmann,
	Kees Cook, linux-tip-commits

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 18:44       ` Matthias Kaehlcke
  2017-05-05 19:30         ` Linus Torvalds
@ 2017-05-05 19:37         ` hpa
  2017-05-05 21:24           ` Matthias Kaehlcke
  1 sibling, 1 reply; 22+ messages in thread
From: hpa @ 2017-05-05 19:37 UTC (permalink / raw)
  To: Matthias Kaehlcke, Ingo Molnar
  Cc: Peter Zijlstra, md, tglx, torvalds, grundler, linux-kernel,
	ghackmann, keescook, linux-tip-commits

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 19:30         ` Linus Torvalds
@ 2017-05-05 20:36           ` Michael Davidson
  2017-05-06  8:16             ` Peter Zijlstra
  2017-05-05 20:52           ` Matthias Kaehlcke
  1 sibling, 1 reply; 22+ messages in thread
From: Michael Davidson @ 2017-05-05 20:36 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Matthias Kaehlcke, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Peter Anvin, grundler, Linux Kernel Mailing List, Greg Hackmann,
	Kees Cook, linux-tip-commits

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 19:30         ` Linus Torvalds
  2017-05-05 20:36           ` Michael Davidson
@ 2017-05-05 20:52           ` Matthias Kaehlcke
  2017-05-06  9:57             ` Ingo Molnar
  1 sibling, 1 reply; 22+ messages in thread
From: Matthias Kaehlcke @ 2017-05-05 20:52 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Ingo Molnar, Peter Zijlstra, Michael Davidson, Thomas Gleixner,
	Peter Anvin, grundler, Linux Kernel Mailing List, ghackmann,
	Kees Cook, linux-tip-commits

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 19:37         ` hpa
@ 2017-05-05 21:24           ` Matthias Kaehlcke
  0 siblings, 0 replies; 22+ messages in thread
From: Matthias Kaehlcke @ 2017-05-05 21:24 UTC (permalink / raw)
  To: hpa
  Cc: Ingo Molnar, Peter Zijlstra, md, tglx, torvalds, grundler,
	linux-kernel, ghackmann, keescook, linux-tip-commits

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 20:36           ` Michael Davidson
@ 2017-05-06  8:16             ` Peter Zijlstra
  2017-05-07 15:42               ` hpa
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2017-05-06  8:16 UTC (permalink / raw)
  To: Michael Davidson
  Cc: Linus Torvalds, Matthias Kaehlcke, Ingo Molnar, Thomas Gleixner,
	Peter Anvin, grundler, Linux Kernel Mailing List, Greg Hackmann,
	Kees Cook, linux-tip-commits

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-05 20:52           ` Matthias Kaehlcke
@ 2017-05-06  9:57             ` Ingo Molnar
  0 siblings, 0 replies; 22+ messages in thread
From: Ingo Molnar @ 2017-05-06  9:57 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Linus Torvalds, Peter Zijlstra, Michael Davidson,
	Thomas Gleixner, Peter Anvin, grundler,
	Linux Kernel Mailing List, ghackmann, Kees Cook,
	linux-tip-commits


* 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility
  2017-05-06  8:16             ` Peter Zijlstra
@ 2017-05-07 15:42               ` hpa
  0 siblings, 0 replies; 22+ messages in thread
From: hpa @ 2017-05-07 15:42 UTC (permalink / raw)
  To: Peter Zijlstra, Michael Davidson
  Cc: Linus Torvalds, Matthias Kaehlcke, Ingo Molnar, Thomas Gleixner,
	grundler, Linux Kernel Mailing List, Greg Hackmann, Kees Cook,
	linux-tip-commits

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
  2017-04-26 21:29       ` Greg Hackmann
@ 2017-04-29 21:23         ` H. Peter Anvin
  0 siblings, 0 replies; 22+ messages in thread
From: H. Peter Anvin @ 2017-04-29 21:23 UTC (permalink / raw)
  To: Greg Hackmann, Kees Cook
  Cc: Matthias Kaehlcke, Thomas Gleixner, Ingo Molnar, x86, LKML,
	Grant Grundler, Michael Davidson

On 04/26/17 14:29, Greg Hackmann wrote:
> On 04/26/2017 02:24 PM, hpa@zytor.com wrote:
>>>> This really feels like a "fix your compiler" issue.
>>>
>>> We already use the other forms, what's so bad about adding mul too?
>>> And if this lets us build under clang, all the better.
>>>
>>> -Kees
>>
>> It's not bad per se, but if this doesn't eventually gets fixed in
>> clang we'll have no end of this crap.
>>
> 
> AIUI the "problem" is that clang is spilling mix_const into memory
> rather than assigning it to a register.  This is perfectly legal since
> mix_const has a constraint of "rm".  But mul needs a suffix when the
> input is a memory location, since it can't infer the multiplication
> width from the input operand anymore.
> 
> You get the same error message with gcc if you force it to use a memory
> location, by narrowing the constraint from "rm" to "m".

OK, that's a genuine bug.  Please explain that in the comment; it has
nothing to do with clang.

	-hpa

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
  2017-04-26 20:55 [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Matthias Kaehlcke
  2017-04-26 21:00 ` hpa
@ 2017-04-26 22:06 ` Kees Cook
  1 sibling, 0 replies; 22+ messages in thread
From: Kees Cook @ 2017-04-26 22:06 UTC (permalink / raw)
  To: Matthias Kaehlcke, Ingo Molnar
  Cc: Thomas Gleixner, H . Peter Anvin, x86, LKML, Grant Grundler,
	Greg Hackmann, Michael Davidson

On Wed, Apr 26, 2017 at 1:55 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> In difference to gas clang doesn't seem to infer the size from the
> operands. 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>

-Kees

> ---
> Changes in v2:
> - use _ASM_MUL instead of #ifdef
> - updated commit message
>
>  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.rc0.306.g87b477812d-goog
>



-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
  2017-04-26 21:24     ` hpa
@ 2017-04-26 21:29       ` Greg Hackmann
  2017-04-29 21:23         ` H. Peter Anvin
  0 siblings, 1 reply; 22+ messages in thread
From: Greg Hackmann @ 2017-04-26 21:29 UTC (permalink / raw)
  To: hpa, Kees Cook
  Cc: Matthias Kaehlcke, Thomas Gleixner, Ingo Molnar, x86, LKML,
	Grant Grundler, Michael Davidson

On 04/26/2017 02:24 PM, hpa@zytor.com wrote:
>>> This really feels like a "fix your compiler" issue.
>>
>> We already use the other forms, what's so bad about adding mul too?
>> And if this lets us build under clang, all the better.
>>
>> -Kees
>
> It's not bad per se, but if this doesn't eventually gets fixed in clang we'll have no end of this crap.
>

AIUI the "problem" is that clang is spilling mix_const into memory 
rather than assigning it to a register.  This is perfectly legal since 
mix_const has a constraint of "rm".  But mul needs a suffix when the 
input is a memory location, since it can't infer the multiplication 
width from the input operand anymore.

You get the same error message with gcc if you force it to use a memory 
location, by narrowing the constraint from "rm" to "m".

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
  2017-04-26 21:21   ` Kees Cook
@ 2017-04-26 21:24     ` hpa
  2017-04-26 21:29       ` Greg Hackmann
  0 siblings, 1 reply; 22+ messages in thread
From: hpa @ 2017-04-26 21:24 UTC (permalink / raw)
  To: Kees Cook
  Cc: Matthias Kaehlcke, Thomas Gleixner, Ingo Molnar, x86, LKML,
	Grant Grundler, Greg Hackmann, Michael Davidson

On April 26, 2017 2:21:25 PM PDT, Kees Cook <keescook@chromium.org> wrote:
>On Wed, Apr 26, 2017 at 2:00 PM,  <hpa@zytor.com> wrote:
>> On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke
><mka@chromium.org> wrote:
>>>In difference to gas clang doesn't seem to infer the size from the
>>>operands. 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:
>>>- use _ASM_MUL instead of #ifdef
>>>- updated commit message
>>>
>>> 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;
>>
>> This really feels like a "fix your compiler" issue.
>
>We already use the other forms, what's so bad about adding mul too?
>And if this lets us build under clang, all the better.
>
>-Kees

It's not bad per se, but if this doesn't eventually gets fixed in clang we'll have no end of this crap.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
  2017-04-26 21:00 ` hpa
@ 2017-04-26 21:21   ` Kees Cook
  2017-04-26 21:24     ` hpa
  0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2017-04-26 21:21 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Matthias Kaehlcke, Thomas Gleixner, Ingo Molnar, x86, LKML,
	Grant Grundler, Greg Hackmann, Michael Davidson

On Wed, Apr 26, 2017 at 2:00 PM,  <hpa@zytor.com> wrote:
> On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>>In difference to gas clang doesn't seem to infer the size from the
>>operands. 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:
>>- use _ASM_MUL instead of #ifdef
>>- updated commit message
>>
>> 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;
>
> This really feels like a "fix your compiler" issue.

We already use the other forms, what's so bad about adding mul too?
And if this lets us build under clang, all the better.

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
  2017-04-26 20:55 [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Matthias Kaehlcke
@ 2017-04-26 21:00 ` hpa
  2017-04-26 21:21   ` Kees Cook
  2017-04-26 22:06 ` Kees Cook
  1 sibling, 1 reply; 22+ messages in thread
From: hpa @ 2017-04-26 21:00 UTC (permalink / raw)
  To: Matthias Kaehlcke, Thomas Gleixner, Ingo Molnar, Kees Cook
  Cc: x86, linux-kernel, Grant Grundler, Greg Hackmann, Michael Davidson

On April 26, 2017 1:55:04 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
>In difference to gas clang doesn't seem to infer the size from the
>operands. 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:
>- use _ASM_MUL instead of #ifdef
>- updated commit message
>
> 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;

This really feels like a "fix your compiler" issue.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication
@ 2017-04-26 20:55 Matthias Kaehlcke
  2017-04-26 21:00 ` hpa
  2017-04-26 22:06 ` Kees Cook
  0 siblings, 2 replies; 22+ messages in thread
From: Matthias Kaehlcke @ 2017-04-26 20:55 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Kees Cook
  Cc: x86, linux-kernel, Grant Grundler, Greg Hackmann,
	Michael Davidson, Matthias Kaehlcke

In difference to gas clang doesn't seem to infer the size from the
operands. 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:
- use _ASM_MUL instead of #ifdef
- updated commit message

 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.rc0.306.g87b477812d-goog

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-01 22:47 [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Matthias Kaehlcke
2017-05-02  2:08 ` Kees Cook
2017-05-05  8:11 ` [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility tip-bot for Matthias Kaehlcke
2017-05-05 10:25   ` Peter Zijlstra
2017-05-05 17:50     ` Ingo Molnar
2017-05-05 18:22       ` Peter Zijlstra
2017-05-05 18:44       ` Matthias Kaehlcke
2017-05-05 19:30         ` Linus Torvalds
2017-05-05 20:36           ` Michael Davidson
2017-05-06  8:16             ` Peter Zijlstra
2017-05-07 15:42               ` hpa
2017-05-05 20:52           ` Matthias Kaehlcke
2017-05-06  9:57             ` Ingo Molnar
2017-05-05 19:37         ` hpa
2017-05-05 21:24           ` Matthias Kaehlcke
  -- strict thread matches above, loose matches on Subject: below --
2017-04-26 20:55 [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Matthias Kaehlcke
2017-04-26 21:00 ` hpa
2017-04-26 21:21   ` Kees Cook
2017-04-26 21:24     ` hpa
2017-04-26 21:29       ` Greg Hackmann
2017-04-29 21:23         ` H. Peter Anvin
2017-04-26 22:06 ` Kees Cook

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox