Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1631723 > unrolled thread
| Started by | Matthias Kaehlcke <mka@chromium.org> |
|---|---|
| First post | 2017-04-26 23:00 +0200 |
| Last post | 2017-04-27 00:10 +0200 |
| Articles | 7 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Matthias Kaehlcke <mka@chromium.org> - 2017-04-26 23:00 +0200
Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication hpa@zytor.com - 2017-04-26 23:10 +0200
Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Greg Hackmann <ghackmann@google.com> - 2017-04-26 23:30 +0200
Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication "H. Peter Anvin" <hpa@zytor.com> - 2017-04-29 23:40 +0200
Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Kees Cook <keescook@chromium.org> - 2017-04-26 23:30 +0200
Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication hpa@zytor.com - 2017-04-26 23:40 +0200
Re: [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication Kees Cook <keescook@chromium.org> - 2017-04-27 00:10 +0200
| From | Matthias Kaehlcke <mka@chromium.org> |
|---|---|
| Date | 2017-04-26 23:00 +0200 |
| Subject | [PATCH v2] x86/mm/kaslr: Use _ASM_MUL macro for multiplication |
| Message-ID | <tACcx-7k3-3@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | hpa@zytor.com |
|---|---|
| Date | 2017-04-26 23:10 +0200 |
| Message-ID | <tACme-7Du-27@gated-at.bofh.it> |
| In reply to | #1631723 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Greg Hackmann <ghackmann@google.com> |
|---|---|
| Date | 2017-04-26 23:30 +0200 |
| Message-ID | <tACFz-7Oy-3@gated-at.bofh.it> |
| In reply to | #1631727 |
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".
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2017-04-29 23:40 +0200 |
| Message-ID | <tBIfT-2Xk-1@gated-at.bofh.it> |
| In reply to | #1631735 |
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
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-04-26 23:30 +0200 |
| Message-ID | <tACFz-7Oy-5@gated-at.bofh.it> |
| In reply to | #1631727 |
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
[toc] | [prev] | [next] | [standalone]
| From | hpa@zytor.com |
|---|---|
| Date | 2017-04-26 23:40 +0200 |
| Message-ID | <tACFz-7Oy-7@gated-at.bofh.it> |
| In reply to | #1631736 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-04-27 00:10 +0200 |
| Message-ID | <tADii-8v0-21@gated-at.bofh.it> |
| In reply to | #1631723 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web