Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1686045 > unrolled thread

[PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

Back to article view | Back to linux.kernel

Contents

  [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-12 23:30 +0200
    Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-13 00:20 +0200
      Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-13 00:30 +0200
        Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-13 00:40 +0200
          Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-13 00:40 +0200
            Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-13 20:10 +0200
              Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-13 20:50 +0200
                Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-13 21:30 +0200
                  Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Michael Davidson <md@google.com> - 2017-07-13 21:40 +0200
                    Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-13 22:20 +0200
                Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Andrey Rybainin <ryabinin.a.a@gmail.com> - 2017-07-13 22:30 +0200
                  Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-13 22:40 +0200
                    Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-13 23:20 +0200
                      Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-13 23:40 +0200
                        Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-14 00:00 +0200
                  Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-13 23:20 +0200
                    Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Andrey Rybainin <ryabinin.a.a@gmail.com> - 2017-07-13 23:30 +0200
                      Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-13 23:50 +0200
                        Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-14 00:00 +0200

#1686045 — [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

Commit f05058c4d652 supposedly "forces a stack frame to be created before
the inline asm code if CONFIG_FRAME_POINTER is enabled by listing the
stack pointer as an output operand for the get_user() inline assembly
statement.". This doesn't work as intended, at least with gcc v4.9.2 and
x86-64 the generated code is exactly the same with and without the patch.
However clang adds an extra instruction that adjusts %rsp, which ends up
causing double faults all over the place.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/x86/include/asm/uaccess.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 476ea27f490b..9ec2beab73df 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -161,11 +161,10 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 ({									\
 	int __ret_gu;							\
 	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
-	register void *__sp asm(_ASM_SP);				\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
-	asm volatile("call __get_user_%P4"				\
-		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
+	asm volatile("call __get_user_%P3"				\
+		     : "=a" (__ret_gu), "=r" (__val_gu)			\
 		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
 	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
 	__builtin_expect(__ret_gu, 0);					\
-- 
2.13.2.932.g7449e964c-goog

[toc] | [next] | [standalone]

#1686073 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Wed, Jul 12, 2017 at 02:27:44PM -0700, Matthias Kaehlcke wrote:
> Commit f05058c4d652 supposedly "forces a stack frame to be created before
> the inline asm code if CONFIG_FRAME_POINTER is enabled by listing the
> stack pointer as an output operand for the get_user() inline assembly
> statement.". This doesn't work as intended, at least with gcc v4.9.2 and
> x86-64 the generated code is exactly the same with and without the patch.
> However clang adds an extra instruction that adjusts %rsp, which ends up
> causing double faults all over the place.

I don't think reverting it is the right approach, because that will
still break frame pointers in certain cases.

The original commit probably should have clarified:

 " ... forces a stack frame *if it doesn't already exist*."
 
In *most* cases it will have no effect, as you saw, because users of
get_user() tend to do other function calls beforehand, so they will have
already saved the frame pointer before calling it.

However, that isn't always the case.  We found that certain configs
change GCC's behavior such that, for certain get_user() call sites, the
containing function doesn't saved the frame pointer before inserting
get_user()'s inline asm.

GCC completely ignores inline asm, so it has no idea that it has a call
instruction in it.  So in general, *any* inline asm with a call
instruction needs this constraint, to force the frame pointer to be
saved, if it hasn't already.

This is admittedly an awkward way of achieving this goal, but it's the
only way I know how to do it with GCC.

What extra instruction does clang add?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1686077 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

Hi Josh,

thanks for your prompt reply.

El Wed, Jul 12, 2017 at 05:12:42PM -0500 Josh Poimboeuf ha dit:

> On Wed, Jul 12, 2017 at 02:27:44PM -0700, Matthias Kaehlcke wrote:
> > Commit f05058c4d652 supposedly "forces a stack frame to be created before
> > the inline asm code if CONFIG_FRAME_POINTER is enabled by listing the
> > stack pointer as an output operand for the get_user() inline assembly
> > statement.". This doesn't work as intended, at least with gcc v4.9.2 and
> > x86-64 the generated code is exactly the same with and without the patch.
> > However clang adds an extra instruction that adjusts %rsp, which ends up
> > causing double faults all over the place.
> 
> I don't think reverting it is the right approach, because that will
> still break frame pointers in certain cases.
> 
> The original commit probably should have clarified:
> 
>  " ... forces a stack frame *if it doesn't already exist*."
>
>
> In *most* cases it will have no effect, as you saw, because users of
> get_user() tend to do other function calls beforehand, so they will have
> already saved the frame pointer before calling it.
> 
> However, that isn't always the case.  We found that certain configs
> change GCC's behavior such that, for certain get_user() call sites, the
> containing function doesn't saved the frame pointer before inserting
> get_user()'s inline asm.
> 
> GCC completely ignores inline asm, so it has no idea that it has a call
> instruction in it.  So in general, *any* inline asm with a call
> instruction needs this constraint, to force the frame pointer to be
> saved, if it hasn't already.

Thanks for the clarification!

> This is admittedly an awkward way of achieving this goal, but it's the
> only way I know how to do it with GCC.
> 
> What extra instruction does clang add?

I was looking at the get_user() call in drm_mode_setcrtc(). The code
generated by clang without the patch is:

                        if (get_user(out_id, &set_connectors_ptr[i])) {
ffffffff81386955:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
ffffffff8138695c:       00 
ffffffff8138695d:       49 03 06                add    (%r14),%rax
ffffffff81386960:       e8 2b a5 f0 ff          callq  ffffffff81290e90 <__get_user_4>

And with the patch:

                        if (get_user(out_id, &set_connectors_ptr[i])) {
ffffffff81386a56:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
ffffffff81386a5d:       00 
ffffffff81386a5e:       49 03 06                add    (%r14),%rax
ffffffff81386a61:       48 8b 64 24 28          mov    0x28(%rsp),%rsp
ffffffff81386a66:       e8 15 a5 f0 ff          callq
ffffffff81290f80 <__get_user_4>

[toc] | [prev] | [next] | [standalone]

#1686082 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Wed, Jul 12, 2017 at 03:20:40PM -0700, Matthias Kaehlcke wrote:
> > This is admittedly an awkward way of achieving this goal, but it's the
> > only way I know how to do it with GCC.
> > 
> > What extra instruction does clang add?
> 
> I was looking at the get_user() call in drm_mode_setcrtc(). The code
> generated by clang without the patch is:
> 
>                         if (get_user(out_id, &set_connectors_ptr[i])) {
> ffffffff81386955:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
> ffffffff8138695c:       00 
> ffffffff8138695d:       49 03 06                add    (%r14),%rax
> ffffffff81386960:       e8 2b a5 f0 ff          callq  ffffffff81290e90 <__get_user_4>
> 
> And with the patch:
> 
>                         if (get_user(out_id, &set_connectors_ptr[i])) {
> ffffffff81386a56:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
> ffffffff81386a5d:       00 
> ffffffff81386a5e:       49 03 06                add    (%r14),%rax
> ffffffff81386a61:       48 8b 64 24 28          mov    0x28(%rsp),%rsp
> ffffffff81386a66:       e8 15 a5 f0 ff          callq
> ffffffff81290f80 <__get_user_4>

Hm, that seems odd.  Can you sure the disassembly for the whole
function?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1686090 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Wed, Jul 12, 2017 at 05:35:47PM -0500, Josh Poimboeuf wrote:
> On Wed, Jul 12, 2017 at 03:20:40PM -0700, Matthias Kaehlcke wrote:
> > > This is admittedly an awkward way of achieving this goal, but it's the
> > > only way I know how to do it with GCC.
> > > 
> > > What extra instruction does clang add?
> > 
> > I was looking at the get_user() call in drm_mode_setcrtc(). The code
> > generated by clang without the patch is:
> > 
> >                         if (get_user(out_id, &set_connectors_ptr[i])) {
> > ffffffff81386955:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
> > ffffffff8138695c:       00 
> > ffffffff8138695d:       49 03 06                add    (%r14),%rax
> > ffffffff81386960:       e8 2b a5 f0 ff          callq  ffffffff81290e90 <__get_user_4>
> > 
> > And with the patch:
> > 
> >                         if (get_user(out_id, &set_connectors_ptr[i])) {
> > ffffffff81386a56:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
> > ffffffff81386a5d:       00 
> > ffffffff81386a5e:       49 03 06                add    (%r14),%rax
> > ffffffff81386a61:       48 8b 64 24 28          mov    0x28(%rsp),%rsp
> > ffffffff81386a66:       e8 15 a5 f0 ff          callq
> > ffffffff81290f80 <__get_user_4>
> 
> Hm, that seems odd.  Can you sure the disassembly for the whole
> function?

Er, share :-)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1686824 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Wed, Jul 12, 2017 at 04:22:13PM -0700, Matthias Kaehlcke wrote:
> El Wed, Jul 12, 2017 at 05:36:30PM -0500 Josh Poimboeuf ha dit:
> 
> > On Wed, Jul 12, 2017 at 05:35:47PM -0500, Josh Poimboeuf wrote:
> > > On Wed, Jul 12, 2017 at 03:20:40PM -0700, Matthias Kaehlcke wrote:
> > > > > This is admittedly an awkward way of achieving this goal, but it's the
> > > > > only way I know how to do it with GCC.
> > > > > 
> > > > > What extra instruction does clang add?
> > > > 
> > > > I was looking at the get_user() call in drm_mode_setcrtc(). The code
> > > > generated by clang without the patch is:
> > > > 
> > > >                         if (get_user(out_id, &set_connectors_ptr[i])) {
> > > > ffffffff81386955:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
> > > > ffffffff8138695c:       00 
> > > > ffffffff8138695d:       49 03 06                add    (%r14),%rax
> > > > ffffffff81386960:       e8 2b a5 f0 ff          callq  ffffffff81290e90 <__get_user_4>
> > > > 
> > > > And with the patch:
> > > > 
> > > >                         if (get_user(out_id, &set_connectors_ptr[i])) {
> > > > ffffffff81386a56:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
> > > > ffffffff81386a5d:       00 
> > > > ffffffff81386a5e:       49 03 06                add    (%r14),%rax
> > > > ffffffff81386a61:       48 8b 64 24 28          mov    0x28(%rsp),%rsp
> > > > ffffffff81386a66:       e8 15 a5 f0 ff          callq
> > > > ffffffff81290f80 <__get_user_4>
> > > 
> > > Hm, that seems odd.  Can you sure the disassembly for the whole
> > > function?
> > 
> > Er, share :-)
> 
> Sure, please find below the disassemblies with and without the
> patch. The exact extra instruction differs from the one above, the
> disassembly above is from a debug session with some 'random' kernel
> version (bisect), the ones below from a v4.12ish kernel. At the bottom
> you also find a log of a double faults observed with the patch.
> 
> If you are interested in building the kernel with clang yourself I can
> provide instructions, it is fairly painless nowadays as long as you
> have a recent version of clang (a somewhat older version should also
> do for this issue with some extra kernel patches).

Here's the reason for the double fault.  First it puts zero on the stack
at offset -0x58:

> ffffffff81367616:	31 c0                	xor    %eax,%eax
> ffffffff81367618:	48 89 45 c8          	mov    %rax,-0x38(%rbp)
> ffffffff8136761c:	45 31 ff             	xor    %r15d,%r15d
> ffffffff8136761f:	48 89 45 a8          	mov    %rax,-0x58(%rbp)

Then, later, it copies that zeroed word from the stack to RSP:

> ffffffff81367874:	48 8b 65 a8          	mov    -0x58(%rbp),%rsp

Then it double faults because the call instruction tries to write RIP on
the stack, but RSP is zero:

> ffffffff81367878:	e8 73 26 f1 ff       	callq  ffffffff81279ef0 <__get_user_4>

Then clang tries to put RSP's value on the stack, at the same stack slot
where the original zero was stored (though it never reaches this point):

> ffffffff8136787d:	49 89 d4             	mov    %rdx,%r12
> ffffffff81367880:	48 89 65 a8          	mov    %rsp,-0x58(%rbp)

The panic is consistent with the above.  RIP points to the call
instruction, RSP is zero:

> [    3.798722] PANIC: double fault, error_code: 0x0
> [    3.807387] CPU: 1 PID: 605 Comm: frecon Not tainted 4.12.0-00023-g711d82c128ff #107
> [    3.816040] Hardware name: GOOGLE Squawks, BIOS Google_Squawks.5216.152.76 03/04/2016
> [    3.824792] task: ffff880075b92f00 task.stack: ffffc90000d6c000
> [    3.829599] EXT4-fs (mmcblk0p1): re-mounted. Opts: commit=600,data=ordered
> [    3.839092] RIP: 0010:drm_mode_setcrtc+0x328/0x51f
> [    3.844443] RSP: 0018:0000000000000000 EFLAGS: 00010206
> [    3.850280] RAX: 0000559e707c4d60 RBX: 0000000000000000 RCX: 0000000000000008
> [    3.858253] RDX: 0000000000000001 RSI: ffffc90000d6fcc8 RDI: ffffffff81367805
> [    3.866225] RBP: ffffc90000d6fd90 R08: 00000000014000c0 R09: 0000000000000308
> [    3.874199] R10: 0000000000000300 R11: 0000000000000556 R12: 0000000000000000
> [    3.882163] R13: ffff880077a25000 R14: ffffc90000d6fdd0 R15: 0000000000000000
> [    3.890136] FS:  00007fb2dbd62740(0000) GS:ffff88007ad00000(0000) knlGS:0000000000000000
> [    3.899177] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    3.905595] CR2: fffffffffffffff8 CR3: 000000007596b000 CR4: 00000000001006e0
> [    3.913568] Call Trace:
> [    3.916296] Code: b8 48 8d b5 38 ff ff ff 41 8b 56 08 39 d3 0f 83 85 00 00 00 4c 63 fb 4a 83 24 f8 00 4a 8d 04 bd 00 00 00 00 49 03 06 48 8b 65 a8 <e8> 73 26 f1 ff 49 89 d4 48 89 65 a8 85 c0 0f 85 a0 00 00 00 ba 
> [    3.937440] Kernel panic - not syncing: Machine halted.
> [    3.943268] CPU: 1 PID: 605 Comm: frecon Not tainted 4.12.0-00023-g711d82c128ff #107
> [    3.951921] Hardware name: GOOGLE Squawks, BIOS Google_Squawks.5216.152.76 03/04/2016
> [    3.960671] Call Trace:
> [    3.963398]  <#DF>
> [    3.965637]  __dump_stack+0x19/0x1b
> [    3.969531]  dump_stack+0x42/0x60

clang is obviously getting confused by the RSP output constraint.  I
think it tries to take the constraint literally, since it takes RSP as
an output from the inline asm and stores it on the stack.  However, that
behavior doesn't really make sense for a "register" variable.  It also
doesn't explain why it's zeroing the register out first.

What happens if you try the below patch instead of the revert?  Any
chance the offending instruction goes away?

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 11433f9..beac907 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 	might_fault();							\
 	asm volatile("call __get_user_%P4"				\
 		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
-		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
+		     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp));	\
 	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
 	__builtin_expect(__ret_gu, 0);					\
 })

[toc] | [prev] | [next] | [standalone]

#1686842 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

El Thu, Jul 13, 2017 at 01:00:01PM -0500 Josh Poimboeuf ha dit:

> On Wed, Jul 12, 2017 at 04:22:13PM -0700, Matthias Kaehlcke wrote:
> > El Wed, Jul 12, 2017 at 05:36:30PM -0500 Josh Poimboeuf ha dit:
> > 
> > > On Wed, Jul 12, 2017 at 05:35:47PM -0500, Josh Poimboeuf wrote:
> > > > On Wed, Jul 12, 2017 at 03:20:40PM -0700, Matthias Kaehlcke wrote:
> > > > > > This is admittedly an awkward way of achieving this goal, but it's the
> > > > > > only way I know how to do it with GCC.
> > > > > > 
> > > > > > What extra instruction does clang add?
> > > > > 
> > > > > I was looking at the get_user() call in drm_mode_setcrtc(). The code
> > > > > generated by clang without the patch is:
> > > > > 
> > > > >                         if (get_user(out_id, &set_connectors_ptr[i])) {
> > > > > ffffffff81386955:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
> > > > > ffffffff8138695c:       00 
> > > > > ffffffff8138695d:       49 03 06                add    (%r14),%rax
> > > > > ffffffff81386960:       e8 2b a5 f0 ff          callq  ffffffff81290e90 <__get_user_4>
> > > > > 
> > > > > And with the patch:
> > > > > 
> > > > >                         if (get_user(out_id, &set_connectors_ptr[i])) {
> > > > > ffffffff81386a56:       4a 8d 04 bd 00 00 00    lea    0x0(,%r15,4),%rax
> > > > > ffffffff81386a5d:       00 
> > > > > ffffffff81386a5e:       49 03 06                add    (%r14),%rax
> > > > > ffffffff81386a61:       48 8b 64 24 28          mov    0x28(%rsp),%rsp
> > > > > ffffffff81386a66:       e8 15 a5 f0 ff          callq
> > > > > ffffffff81290f80 <__get_user_4>
> > > > 
> > > > Hm, that seems odd.  Can you sure the disassembly for the whole
> > > > function?
> > > 
> > > Er, share :-)
> > 
> > Sure, please find below the disassemblies with and without the
> > patch. The exact extra instruction differs from the one above, the
> > disassembly above is from a debug session with some 'random' kernel
> > version (bisect), the ones below from a v4.12ish kernel. At the bottom
> > you also find a log of a double faults observed with the patch.
> > 
> > If you are interested in building the kernel with clang yourself I can
> > provide instructions, it is fairly painless nowadays as long as you
> > have a recent version of clang (a somewhat older version should also
> > do for this issue with some extra kernel patches).
> 
> Here's the reason for the double fault.  First it puts zero on the stack
> at offset -0x58:
> 
> > ffffffff81367616:	31 c0                	xor    %eax,%eax
> > ffffffff81367618:	48 89 45 c8          	mov    %rax,-0x38(%rbp)
> > ffffffff8136761c:	45 31 ff             	xor    %r15d,%r15d
> > ffffffff8136761f:	48 89 45 a8          	mov    %rax,-0x58(%rbp)
> 
> Then, later, it copies that zeroed word from the stack to RSP:
> 
> > ffffffff81367874:	48 8b 65 a8          	mov    -0x58(%rbp),%rsp
> 
> Then it double faults because the call instruction tries to write RIP on
> the stack, but RSP is zero:
> 
> > ffffffff81367878:	e8 73 26 f1 ff       	callq  ffffffff81279ef0 <__get_user_4>
> 
> Then clang tries to put RSP's value on the stack, at the same stack slot
> where the original zero was stored (though it never reaches this point):
> 
> > ffffffff8136787d:	49 89 d4             	mov    %rdx,%r12
> > ffffffff81367880:	48 89 65 a8          	mov    %rsp,-0x58(%rbp)
> 
> The panic is consistent with the above.  RIP points to the call
> instruction, RSP is zero:
> 
> > [    3.798722] PANIC: double fault, error_code: 0x0
> > [    3.807387] CPU: 1 PID: 605 Comm: frecon Not tainted 4.12.0-00023-g711d82c128ff #107
> > [    3.816040] Hardware name: GOOGLE Squawks, BIOS Google_Squawks.5216.152.76 03/04/2016
> > [    3.824792] task: ffff880075b92f00 task.stack: ffffc90000d6c000
> > [    3.829599] EXT4-fs (mmcblk0p1): re-mounted. Opts: commit=600,data=ordered
> > [    3.839092] RIP: 0010:drm_mode_setcrtc+0x328/0x51f
> > [    3.844443] RSP: 0018:0000000000000000 EFLAGS: 00010206
> > [    3.850280] RAX: 0000559e707c4d60 RBX: 0000000000000000 RCX: 0000000000000008
> > [    3.858253] RDX: 0000000000000001 RSI: ffffc90000d6fcc8 RDI: ffffffff81367805
> > [    3.866225] RBP: ffffc90000d6fd90 R08: 00000000014000c0 R09: 0000000000000308
> > [    3.874199] R10: 0000000000000300 R11: 0000000000000556 R12: 0000000000000000
> > [    3.882163] R13: ffff880077a25000 R14: ffffc90000d6fdd0 R15: 0000000000000000
> > [    3.890136] FS:  00007fb2dbd62740(0000) GS:ffff88007ad00000(0000) knlGS:0000000000000000
> > [    3.899177] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    3.905595] CR2: fffffffffffffff8 CR3: 000000007596b000 CR4: 00000000001006e0
> > [    3.913568] Call Trace:
> > [    3.916296] Code: b8 48 8d b5 38 ff ff ff 41 8b 56 08 39 d3 0f 83 85 00 00 00 4c 63 fb 4a 83 24 f8 00 4a 8d 04 bd 00 00 00 00 49 03 06 48 8b 65 a8 <e8> 73 26 f1 ff 49 89 d4 48 89 65 a8 85 c0 0f 85 a0 00 00 00 ba 
> > [    3.937440] Kernel panic - not syncing: Machine halted.
> > [    3.943268] CPU: 1 PID: 605 Comm: frecon Not tainted 4.12.0-00023-g711d82c128ff #107
> > [    3.951921] Hardware name: GOOGLE Squawks, BIOS Google_Squawks.5216.152.76 03/04/2016
> > [    3.960671] Call Trace:
> > [    3.963398]  <#DF>
> > [    3.965637]  __dump_stack+0x19/0x1b
> > [    3.969531]  dump_stack+0x42/0x60
> 
> clang is obviously getting confused by the RSP output constraint.  I
> think it tries to take the constraint literally, since it takes RSP as
> an output from the inline asm and stores it on the stack.  However, that
> behavior doesn't really make sense for a "register" variable.  It also
> doesn't explain why it's zeroing the register out first.

Thanks for your analysis!

> What happens if you try the below patch instead of the revert?  Any
> chance the offending instruction goes away?
> 
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 11433f9..beac907 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>  	might_fault();							\
>  	asm volatile("call __get_user_%P4"				\
>  		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
> -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> +		     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp));	\
>  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
>  	__builtin_expect(__ret_gu, 0);					\
>  })

The generated code is basically the same, only that now the value from
the stack is stored in a register and written twice to RSP:

ffffffff813676ba:       31 c0                   xor    %eax,%eax
ffffffff813676bc:       48 89 45 c8             mov    %rax,-0x38(%rbp)
ffffffff813676c0:       45 31 ff                xor    %r15d,%r15d
ffffffff813676c3:       48 89 45 a8             mov    %rax,-0x58(%rbp)
...
ffffffff81367918:       48 8b 4d a8             mov    -0x58(%rbp),%rcx
ffffffff8136791c:       48 89 cc                mov    %rcx,%rsp
ffffffff8136791f:       48 89 cc                mov    %rcx,%rsp
ffffffff81367922:       e8 69 26 f1 ff          callq  ffffffff81279f90 <__get_user_4>

[toc] | [prev] | [next] | [standalone]

#1686858 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Thu, Jul 13, 2017 at 11:47:48AM -0700, Matthias Kaehlcke wrote:
> > What happens if you try the below patch instead of the revert?  Any
> > chance the offending instruction goes away?
> > 
> > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> > index 11433f9..beac907 100644
> > --- a/arch/x86/include/asm/uaccess.h
> > +++ b/arch/x86/include/asm/uaccess.h
> > @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> >  	might_fault();							\
> >  	asm volatile("call __get_user_%P4"				\
> >  		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
> > -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> > +		     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp));	\
> >  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
> >  	__builtin_expect(__ret_gu, 0);					\
> >  })
> 
> The generated code is basically the same, only that now the value from
> the stack is stored in a register and written twice to RSP:
> 
> ffffffff813676ba:       31 c0                   xor    %eax,%eax
> ffffffff813676bc:       48 89 45 c8             mov    %rax,-0x38(%rbp)
> ffffffff813676c0:       45 31 ff                xor    %r15d,%r15d
> ffffffff813676c3:       48 89 45 a8             mov    %rax,-0x58(%rbp)
> ...
> ffffffff81367918:       48 8b 4d a8             mov    -0x58(%rbp),%rcx
> ffffffff8136791c:       48 89 cc                mov    %rcx,%rsp
> ffffffff8136791f:       48 89 cc                mov    %rcx,%rsp
> ffffffff81367922:       e8 69 26 f1 ff          callq  ffffffff81279f90 <__get_user_4>

LOL.  Why corrupt the stack pointer with a single instruction (reading a
zero from memory, no less) when you can instead do it with three
instructions, including two duplicates?

Anyway this seems like a clang bug to me.  If I specify RSP as an input
register then the compiler shouldn't overwrite it first.  For that
matter it has no reason to overwrite it if it's an output register
either.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1686862 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Thu, Jul 13, 2017 at 12:25 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:

>
> Anyway this seems like a clang bug to me.  If I specify RSP as an input
> register then the compiler shouldn't overwrite it first.  For that
> matter it has no reason to overwrite it if it's an output register
> either.
>

It's certainly  a difference in behavior between clang and gcc.

My question is whether this particular construct is really a
"supported" (or, at least, reasonably guaranteed) way of forcing gcc
to create a stack frame if none exists. or whether it is something
that "just happens to work".

If someone could explain the rationale behind *why* this works the way
that it does on gcc that might help convince the clang people that
this is actually a bug rather than just a piece of undefined behavior
which gcc and clang happen to handle differently.

[toc] | [prev] | [next] | [standalone]

#1686876 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Thu, Jul 13, 2017 at 12:38:32PM -0700, Michael Davidson wrote:
> On Thu, Jul 13, 2017 at 12:25 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> >
> > Anyway this seems like a clang bug to me.  If I specify RSP as an input
> > register then the compiler shouldn't overwrite it first.  For that
> > matter it has no reason to overwrite it if it's an output register
> > either.
> >
> 
> It's certainly  a difference in behavior between clang and gcc.
> 
> My question is whether this particular construct is really a
> "supported" (or, at least, reasonably guaranteed) way of forcing gcc
> to create a stack frame if none exists. or whether it is something
> that "just happens to work".
> 
> If someone could explain the rationale behind *why* this works the way
> that it does on gcc that might help convince the clang people that
> this is actually a bug rather than just a piece of undefined behavior
> which gcc and clang happen to handle differently.

Disclaimer: I'm no compiler expert, and there are usually a variety of
opinions about compiler undefined behavior.  So it would probably be
good for real compiler people to participate in the discussion.

But I think there are two separate issues here.

1) The first issue is whether it's supported behavior to specify RSP as
   an output constraint in order to force GCC to create a stack frame.
   As far as I know, this is a quirk of GCC, and not really considered
   defined behavior.

   However, the idea was suggested by some GCC developers:

     https://gcc.gnu.org/ml/gcc/2015-07/msg00079.html

   So at least it seems to be endorsed by GCC to some degree.  If you
   need details on why it works, that thread has the details.

2) The second issue is whether clang should corrupt RSP.  I don't see a
   reason for clang to do that.  IMO, when using a local register
   variable as an input or output to inline asm, the compiler should
   leave the contents of the register alone.

   FWIW, my reading of the GCC manual seems to support that:

     https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html#Local-Register-Variables


-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1686887 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On 07/13/2017 09:47 PM, Matthias Kaehlcke wrote:

> Thanks for your analysis!
> 
>> What happens if you try the below patch instead of the revert?  Any
>> chance the offending instruction goes away?
>>
>> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
>> index 11433f9..beac907 100644
>> --- a/arch/x86/include/asm/uaccess.h
>> +++ b/arch/x86/include/asm/uaccess.h
>> @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>>  	might_fault();							\
>>  	asm volatile("call __get_user_%P4"				\
>>  		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
>> -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
>> +		     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp));	\
>>  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
>>  	__builtin_expect(__ret_gu, 0);					\
>>  })
> 
> The generated code is basically the same, only that now the value from
> the stack is stored in a register and written twice to RSP:
> 

AFAIR clang works much better with global named registers.
Could you check if the patch bellow helps?


---
 arch/x86/include/asm/uaccess.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a059aac9e937..121204387978 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -157,15 +157,18 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
  * Clang/LLVM cares about the size of the register, but still wants
  * the base register for something that ends up being a pair.
  */
+
+register unsigned long __current_sp asm(_ASM_SP);
+
 #define get_user(x, ptr)						\
 ({									\
 	int __ret_gu;							\
 	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
-	register void *__sp asm(_ASM_SP);				\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
 	asm volatile("call __get_user_%P4"				\
-		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
+		     : "=a" (__ret_gu), "=r" (__val_gu), 		\
+		       "+r" (__current_sp)				\
 		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
 	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
 	__builtin_expect(__ret_gu, 0);					\
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1686897 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Thu, Jul 13, 2017 at 11:20:04PM +0300, Andrey Rybainin wrote:
> On 07/13/2017 09:47 PM, Matthias Kaehlcke wrote:
> 
> > Thanks for your analysis!
> > 
> >> What happens if you try the below patch instead of the revert?  Any
> >> chance the offending instruction goes away?
> >>
> >> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> >> index 11433f9..beac907 100644
> >> --- a/arch/x86/include/asm/uaccess.h
> >> +++ b/arch/x86/include/asm/uaccess.h
> >> @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> >>  	might_fault();							\
> >>  	asm volatile("call __get_user_%P4"				\
> >>  		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
> >> -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> >> +		     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp));	\
> >>  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
> >>  	__builtin_expect(__ret_gu, 0);					\
> >>  })
> > 
> > The generated code is basically the same, only that now the value from
> > the stack is stored in a register and written twice to RSP:
> > 
> 
> AFAIR clang works much better with global named registers.
> Could you check if the patch bellow helps?

And yet another one to try (clobbering sp) :-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 11433f9..21f0c39 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -166,12 +166,12 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 ({									\
 	int __ret_gu;							\
 	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
-	register void *__sp asm(_ASM_SP);				\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
-	asm volatile("call __get_user_%P4"				\
-		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
-		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
+	asm volatile("call __get_user_%P3"				\
+		     : "=a" (__ret_gu), "=r" (__val_gu)			\
+		     : "0" (ptr), "i" (sizeof(*(ptr)))			\
+		     : "sp");						\
 	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
 	__builtin_expect(__ret_gu, 0);					\
 })

[toc] | [prev] | [next] | [standalone]

#1686919 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

El Thu, Jul 13, 2017 at 03:34:16PM -0500 Josh Poimboeuf ha dit:

> On Thu, Jul 13, 2017 at 11:20:04PM +0300, Andrey Rybainin wrote:
> > On 07/13/2017 09:47 PM, Matthias Kaehlcke wrote:
> > 
> > > Thanks for your analysis!
> > > 
> > >> What happens if you try the below patch instead of the revert?  Any
> > >> chance the offending instruction goes away?
> > >>
> > >> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> > >> index 11433f9..beac907 100644
> > >> --- a/arch/x86/include/asm/uaccess.h
> > >> +++ b/arch/x86/include/asm/uaccess.h
> > >> @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> > >>  	might_fault();							\
> > >>  	asm volatile("call __get_user_%P4"				\
> > >>  		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
> > >> -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> > >> +		     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp));	\
> > >>  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
> > >>  	__builtin_expect(__ret_gu, 0);					\
> > >>  })
> > > 
> > > The generated code is basically the same, only that now the value from
> > > the stack is stored in a register and written twice to RSP:
> > > 
> > 
> > AFAIR clang works much better with global named registers.
> > Could you check if the patch bellow helps?
> 
> And yet another one to try (clobbering sp) :-)
> 
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 11433f9..21f0c39 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -166,12 +166,12 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>  ({									\
>  	int __ret_gu;							\
>  	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
> -	register void *__sp asm(_ASM_SP);				\
>  	__chk_user_ptr(ptr);						\
>  	might_fault();							\
> -	asm volatile("call __get_user_%P4"				\
> -		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
> -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> +	asm volatile("call __get_user_%P3"				\
> +		     : "=a" (__ret_gu), "=r" (__val_gu)			\
> +		     : "0" (ptr), "i" (sizeof(*(ptr)))			\
> +		     : "sp");						\
>  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
>  	__builtin_expect(__ret_gu, 0);					\
>  })

This compiles with both gcc and clang, clang does not corrupt the
stack pointer. I wouldn't be able to tell though if it forces a stack
frame if it doesn't already exist, as the original patch intends.

[toc] | [prev] | [next] | [standalone]

#1686937 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Thu, Jul 13, 2017 at 02:12:45PM -0700, Matthias Kaehlcke wrote:
> El Thu, Jul 13, 2017 at 03:34:16PM -0500 Josh Poimboeuf ha dit:
> > And yet another one to try (clobbering sp) :-)
> > 
> > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> > index 11433f9..21f0c39 100644
> > --- a/arch/x86/include/asm/uaccess.h
> > +++ b/arch/x86/include/asm/uaccess.h
> > @@ -166,12 +166,12 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> >  ({									\
> >  	int __ret_gu;							\
> >  	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
> > -	register void *__sp asm(_ASM_SP);				\
> >  	__chk_user_ptr(ptr);						\
> >  	might_fault();							\
> > -	asm volatile("call __get_user_%P4"				\
> > -		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
> > -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> > +	asm volatile("call __get_user_%P3"				\
> > +		     : "=a" (__ret_gu), "=r" (__val_gu)			\
> > +		     : "0" (ptr), "i" (sizeof(*(ptr)))			\
> > +		     : "sp");						\
> >  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
> >  	__builtin_expect(__ret_gu, 0);					\
> >  })
> 
> This compiles with both gcc and clang, clang does not corrupt the
> stack pointer. I wouldn't be able to tell though if it forces a stack
> frame if it doesn't already exist, as the original patch intends.

Whether it forces the stack frame on clang is a very minor issue
compared to the double fault.  That really only matters when you want to
use CONFIG_STACK_VALIDATION to get 100% reliable stacktraces with frame
pointers.  And that feature is currently very GCC-specific.  So you
probably don't need to worry about that for now, at least until you want
to do live patching with a clang-compiled kernel.

IIRC, clobbering SP does at least force the stack frame on GCC, though I
need to double check that.  I can try to work up an official patch in
the next week or so (need to do some testing first).

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1686945 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

El Thu, Jul 13, 2017 at 04:34:06PM -0500 Josh Poimboeuf ha dit:

> On Thu, Jul 13, 2017 at 02:12:45PM -0700, Matthias Kaehlcke wrote:
> > El Thu, Jul 13, 2017 at 03:34:16PM -0500 Josh Poimboeuf ha dit:
> > > And yet another one to try (clobbering sp) :-)
> > > 
> > > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> > > index 11433f9..21f0c39 100644
> > > --- a/arch/x86/include/asm/uaccess.h
> > > +++ b/arch/x86/include/asm/uaccess.h
> > > @@ -166,12 +166,12 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> > >  ({									\
> > >  	int __ret_gu;							\
> > >  	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
> > > -	register void *__sp asm(_ASM_SP);				\
> > >  	__chk_user_ptr(ptr);						\
> > >  	might_fault();							\
> > > -	asm volatile("call __get_user_%P4"				\
> > > -		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
> > > -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> > > +	asm volatile("call __get_user_%P3"				\
> > > +		     : "=a" (__ret_gu), "=r" (__val_gu)			\
> > > +		     : "0" (ptr), "i" (sizeof(*(ptr)))			\
> > > +		     : "sp");						\
> > >  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
> > >  	__builtin_expect(__ret_gu, 0);					\
> > >  })
> > 
> > This compiles with both gcc and clang, clang does not corrupt the
> > stack pointer. I wouldn't be able to tell though if it forces a stack
> > frame if it doesn't already exist, as the original patch intends.
> 
> Whether it forces the stack frame on clang is a very minor issue
> compared to the double fault.

I totally agree, I was mainly concerned about not breaking the
solution that currently works with gcc.

> That really only matters when you want to use
> CONFIG_STACK_VALIDATION to get 100% reliable stacktraces with frame
> pointers.  And that feature is currently very GCC-specific.  So you
> probably don't need to worry about that for now, at least until you want
> to do live patching with a clang-compiled kernel.

Eventually I expect that there will be interest in live patching
clang-compiled kernels, however at this stage it probably isn't an
overly important feature.

> IIRC, clobbering SP does at least force the stack frame on GCC, though I
> need to double check that.  I can try to work up an official patch in
> the next week or so (need to do some testing first).

Sounds great.

Thanks again for looking into this and coming up with a solution!

Matthias

[toc] | [prev] | [next] | [standalone]

#1686923 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

El Thu, Jul 13, 2017 at 11:20:04PM +0300 Andrey Rybainin ha dit:

> On 07/13/2017 09:47 PM, Matthias Kaehlcke wrote:
>
> > Thanks for your analysis!
> >
> >> What happens if you try the below patch instead of the revert?  Any
> >> chance the offending instruction goes away?
> >>
> >> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> >> index 11433f9..beac907 100644
> >> --- a/arch/x86/include/asm/uaccess.h
> >> +++ b/arch/x86/include/asm/uaccess.h
> >> @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> >>   might_fault(); \
> >>   asm volatile("call __get_user_%P4" \
> >>       : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \
> >> -     : "0" (ptr), "i" (sizeof(*(ptr)))); \
> >> +     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp)); \
> >>   (x) = (__force __typeof__(*(ptr))) __val_gu; \
> >>   __builtin_expect(__ret_gu, 0); \
> >>  })
> >
> > The generated code is basically the same, only that now the value from
> > the stack is stored in a register and written twice to RSP:
> >
>
> AFAIR clang works much better with global named registers.
> Could you check if the patch bellow helps?
>
>
> ---
>  arch/x86/include/asm/uaccess.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index a059aac9e937..121204387978 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -157,15 +157,18 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>   * Clang/LLVM cares about the size of the register, but still wants
>   * the base register for something that ends up being a pair.
>   */
> +
> +register unsigned long __current_sp asm(_ASM_SP);
> +
>  #define get_user(x, ptr) \
>  ({ \
>   int __ret_gu; \
>   register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
> - register void *__sp asm(_ASM_SP); \
>   __chk_user_ptr(ptr); \
>   might_fault(); \
>   asm volatile("call __get_user_%P4" \
> -     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \
> +     : "=a" (__ret_gu), "=r" (__val_gu), \
> +       "+r" (__current_sp) \
>       : "0" (ptr), "i" (sizeof(*(ptr)))); \
>   (x) = (__force __typeof__(*(ptr))) __val_gu; \
>   __builtin_expect(__ret_gu, 0); \

Thanks for the suggestion, however it fails to build with both gcc and clang:

fs/ioctl.c:585:6: error: use of undeclared identifier '__current_sp'
        if (get_user(count, &argp->dest_count)) {
            ^
arch/x86/include/asm/uaccess.h:168:16: note: expanded from macro 'get_user'
                       "+r" (__current_sp)
      \

The references I found refer to __current_sp as an intrinsic function
for ARM32.

[toc] | [prev] | [next] | [standalone]

#1686934 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On 07/14/2017 12:14 AM, Matthias Kaehlcke wrote:
> El Thu, Jul 13, 2017 at 11:20:04PM +0300 Andrey Rybainin ha dit:
> 
>> On 07/13/2017 09:47 PM, Matthias Kaehlcke wrote:
>>
>>> Thanks for your analysis!
>>>
>>>> What happens if you try the below patch instead of the revert?  Any
>>>> chance the offending instruction goes away?
>>>>
>>>> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
>>>> index 11433f9..beac907 100644
>>>> --- a/arch/x86/include/asm/uaccess.h
>>>> +++ b/arch/x86/include/asm/uaccess.h
>>>> @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>>>>   might_fault(); \
>>>>   asm volatile("call __get_user_%P4" \
>>>>       : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \
>>>> -     : "0" (ptr), "i" (sizeof(*(ptr)))); \
>>>> +     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp)); \
>>>>   (x) = (__force __typeof__(*(ptr))) __val_gu; \
>>>>   __builtin_expect(__ret_gu, 0); \
>>>>  })
>>>
>>> The generated code is basically the same, only that now the value from
>>> the stack is stored in a register and written twice to RSP:
>>>
>>
>> AFAIR clang works much better with global named registers.
>> Could you check if the patch bellow helps?
>>
>>
>> ---
>>  arch/x86/include/asm/uaccess.h | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
>> index a059aac9e937..121204387978 100644
>> --- a/arch/x86/include/asm/uaccess.h
>> +++ b/arch/x86/include/asm/uaccess.h
>> @@ -157,15 +157,18 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>>   * Clang/LLVM cares about the size of the register, but still wants
>>   * the base register for something that ends up being a pair.
>>   */
>> +
>> +register unsigned long __current_sp asm(_ASM_SP);
>> +
>>  #define get_user(x, ptr) \
>>  ({ \
>>   int __ret_gu; \
>>   register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
>> - register void *__sp asm(_ASM_SP); \
>>   __chk_user_ptr(ptr); \
>>   might_fault(); \
>>   asm volatile("call __get_user_%P4" \
>> -     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \
>> +     : "=a" (__ret_gu), "=r" (__val_gu), \
>> +       "+r" (__current_sp) \
>>       : "0" (ptr), "i" (sizeof(*(ptr)))); \
>>   (x) = (__force __typeof__(*(ptr))) __val_gu; \
>>   __builtin_expect(__ret_gu, 0); \
> 
> Thanks for the suggestion, however it fails to build with both gcc and clang:
> 
> fs/ioctl.c:585:6: error: use of undeclared identifier '__current_sp'
>         if (get_user(count, &argp->dest_count)) {
>             ^
> arch/x86/include/asm/uaccess.h:168:16: note: expanded from macro 'get_user'
>                        "+r" (__current_sp)
>       \
> 
> The references I found refer to __current_sp as an intrinsic function
> for ARM32.

What? __current_sp declared right above get_user() as "register unsigned long __current_sp asm(_ASM_SP);" 
Did you actually applied my patch or you just modified the code yourself but have missed
"register unsigned long __current_sp asm(_ASM_SP);" ?

FWIW patch works (builds) for me with gcc.

[toc] | [prev] | [next] | [standalone]

#1686943 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

El Fri, Jul 14, 2017 at 12:25:42AM +0300 Andrey Rybainin ha dit:

> 
> 
> On 07/14/2017 12:14 AM, Matthias Kaehlcke wrote:
> > El Thu, Jul 13, 2017 at 11:20:04PM +0300 Andrey Rybainin ha dit:
> > 
> >> On 07/13/2017 09:47 PM, Matthias Kaehlcke wrote:
> >>
> >>> Thanks for your analysis!
> >>>
> >>>> What happens if you try the below patch instead of the revert?  Any
> >>>> chance the offending instruction goes away?
> >>>>
> >>>> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> >>>> index 11433f9..beac907 100644
> >>>> --- a/arch/x86/include/asm/uaccess.h
> >>>> +++ b/arch/x86/include/asm/uaccess.h
> >>>> @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> >>>>   might_fault(); \
> >>>>   asm volatile("call __get_user_%P4" \
> >>>>       : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \
> >>>> -     : "0" (ptr), "i" (sizeof(*(ptr)))); \
> >>>> +     : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp)); \
> >>>>   (x) = (__force __typeof__(*(ptr))) __val_gu; \
> >>>>   __builtin_expect(__ret_gu, 0); \
> >>>>  })
> >>>
> >>> The generated code is basically the same, only that now the value from
> >>> the stack is stored in a register and written twice to RSP:
> >>>
> >>
> >> AFAIR clang works much better with global named registers.
> >> Could you check if the patch bellow helps?
> >>
> >>
> >> ---
> >>  arch/x86/include/asm/uaccess.h | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> >> index a059aac9e937..121204387978 100644
> >> --- a/arch/x86/include/asm/uaccess.h
> >> +++ b/arch/x86/include/asm/uaccess.h
> >> @@ -157,15 +157,18 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> >>   * Clang/LLVM cares about the size of the register, but still wants
> >>   * the base register for something that ends up being a pair.
> >>   */
> >> +
> >> +register unsigned long __current_sp asm(_ASM_SP);
> >> +
> >>  #define get_user(x, ptr) \
> >>  ({ \
> >>   int __ret_gu; \
> >>   register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
> >> - register void *__sp asm(_ASM_SP); \
> >>   __chk_user_ptr(ptr); \
> >>   might_fault(); \
> >>   asm volatile("call __get_user_%P4" \
> >> -     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \
> >> +     : "=a" (__ret_gu), "=r" (__val_gu), \
> >> +       "+r" (__current_sp) \
> >>       : "0" (ptr), "i" (sizeof(*(ptr)))); \
> >>   (x) = (__force __typeof__(*(ptr))) __val_gu; \
> >>   __builtin_expect(__ret_gu, 0); \
> > 
> > Thanks for the suggestion, however it fails to build with both gcc and clang:
> > 
> > fs/ioctl.c:585:6: error: use of undeclared identifier '__current_sp'
> >         if (get_user(count, &argp->dest_count)) {
> >             ^
> > arch/x86/include/asm/uaccess.h:168:16: note: expanded from macro 'get_user'
> >                        "+r" (__current_sp)
> >       \
> > 
> > The references I found refer to __current_sp as an intrinsic function
> > for ARM32.
> 
> What? __current_sp declared right above get_user() as "register unsigned long __current_sp asm(_ASM_SP);" 
> Did you actually applied my patch or you just modified the code yourself but have missed
> "register unsigned long __current_sp asm(_ASM_SP);" ?

Indeed, since the patch is only a few lines and I had the function
already open in the editor it seemed easier to change the affected
lines than to apply the patch and I missed the definition <:‑|

After adding the missing line the code builds with clang and the stack
pointer is not corrupted.

[toc] | [prev] | [next] | [standalone]

#1686949 — Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

On Thu, Jul 13, 2017 at 02:43:26PM -0700, Matthias Kaehlcke wrote:
> > >> AFAIR clang works much better with global named registers.

Is that a bug or a feature?

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web