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

Groups > linux.kernel > #1692066 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-19 19:50 +0200
    Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-20 00:00 +0200
    Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2017-07-20 12:10 +0200
      Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-20 17:20 +0200
        Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2017-07-20 17:40 +0200
          Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-20 23:00 +0200
            Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2017-07-21 11:20 +0200
              Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-21 15:30 +0200
            Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-29 02:40 +0200
              Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-29 03:00 +0200
              Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-29 03:00 +0200
                Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in  get_user() inline asm" Matthias Kaehlcke <mka@chromium.org> - 2017-07-29 03:10 +0200

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

On Thu, Jul 13, 2017 at 02:57:04PM -0700, Matthias Kaehlcke wrote:
> 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!

After doing some testing, I don't think this approach is going to work
after all.  In addition to forcing the stack frame, it also causes GCC
to add an unnecessary extra instruction to the epilogue of each affected
function:

  lea    -0x10(%rbp),%rsp

We shouldn't be inserting extra instructions like that.  I also don't
think the other suggestion of turning the '__sp' register variable into
a global variable is a very good solution either, as that just wastes
memory for no reason.

It would be nice if both compilers could agree on a way to support this.

-- 
Josh

[toc] | [next] | [standalone]

#1692225

El Wed, Jul 19, 2017 at 12:46:31PM -0500 Josh Poimboeuf ha dit:

> On Thu, Jul 13, 2017 at 02:57:04PM -0700, Matthias Kaehlcke wrote:
> > 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!
> 
> After doing some testing, I don't think this approach is going to work
> after all.  In addition to forcing the stack frame, it also causes GCC
> to add an unnecessary extra instruction to the epilogue of each affected
> function:
> 
>   lea    -0x10(%rbp),%rsp
> 
> We shouldn't be inserting extra instructions like that.  I also don't
> think the other suggestion of turning the '__sp' register variable into
> a global variable is a very good solution either, as that just wastes
> memory for no reason.
> 
> It would be nice if both compilers could agree on a way to support this.

Thanks for the update, Josh.

I will ask compiler folks to bring this up with LLVM.

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

#1692721

2017-07-19 20:46 GMT+03:00 Josh Poimboeuf <jpoimboe@redhat.com>:

>
> After doing some testing, I don't think this approach is going to work
> after all.  In addition to forcing the stack frame, it also causes GCC
> to add an unnecessary extra instruction to the epilogue of each affected
> function:
>
>   lea    -0x10(%rbp),%rsp
>
> We shouldn't be inserting extra instructions like that.  I also don't
> think the other suggestion of turning the '__sp' register variable into
> a global variable is a very good solution either, as that just wastes
> memory for no reason.
>

Wastes memory? How is that wastes memory? That doesn't make any sense.

> It would be nice if both compilers could agree on a way to support this.
>
> --
> Josh

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

#1693020

On Thu, Jul 20, 2017 at 01:01:39PM +0300, Andrey Ryabinin wrote:
> 2017-07-19 20:46 GMT+03:00 Josh Poimboeuf <jpoimboe@redhat.com>:
> 
> >
> > After doing some testing, I don't think this approach is going to work
> > after all.  In addition to forcing the stack frame, it also causes GCC
> > to add an unnecessary extra instruction to the epilogue of each affected
> > function:
> >
> >   lea    -0x10(%rbp),%rsp
> >
> > We shouldn't be inserting extra instructions like that.  I also don't
> > think the other suggestion of turning the '__sp' register variable into
> > a global variable is a very good solution either, as that just wastes
> > memory for no reason.
> >
> 
> Wastes memory? How is that wastes memory? That doesn't make any sense.

Yes, you're right, that doesn't make any sense.  I think I was trying to
wrap my head around what it means to have a global register variable --
in a header file no less -- and why clang would treat that differently
than a local register variable.

-- 
Josh

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

#1693037

2017-07-20 18:18 GMT+03:00 Josh Poimboeuf <jpoimboe@redhat.com>:
> On Thu, Jul 20, 2017 at 01:01:39PM +0300, Andrey Ryabinin wrote:
>> 2017-07-19 20:46 GMT+03:00 Josh Poimboeuf <jpoimboe@redhat.com>:
>>
>> >
>> > After doing some testing, I don't think this approach is going to work
>> > after all.  In addition to forcing the stack frame, it also causes GCC
>> > to add an unnecessary extra instruction to the epilogue of each affected
>> > function:
>> >
>> >   lea    -0x10(%rbp),%rsp
>> >
>> > We shouldn't be inserting extra instructions like that.  I also don't
>> > think the other suggestion of turning the '__sp' register variable into
>> > a global variable is a very good solution either, as that just wastes
>> > memory for no reason.
>> >
>>
>> Wastes memory? How is that wastes memory? That doesn't make any sense.
>
> Yes, you're right, that doesn't make any sense.  I think I was trying to
> wrap my head around what it means to have a global register variable --
> in a header file no less -- and why clang would treat that differently
> than a local register variable.
>

FWIW bellow is my understanding of what's going on.

It seems clang treats local named register almost the same as ordinary
local variables.
The only difference is that before reading the register variable clang
puts variable's value into the specified register.

So clang just assigns stack slot for the variable __sp where it's
going to keep variable's value.
But since __sp is unitialized (we haven't assign anything to it), the
value of the __sp is some garbage from stack.
inline asm specifies __sp as input, so clang assumes that it have to
load __sp into 'rsp' because inline asm is going to use
it. And it just loads garbage from stack into 'rsp'

In fact, such behavior (I mean storing the value on stack and loading
into reg before the use) is very useful.
Clang's behavior allows to keep the value assigned to the
call-clobbered register across the function calls.

Unlike clang, gcc assigns value to the register right away and doesn't
store the value anywhere else. So if the reg is
call clobbered register you have to be absolutely sure that there is
no subsequent function call that might clobber the register.

E.g. see some real examples
https://patchwork.kernel.org/patch/4111971/ or 98d4ded60bda("msm: scm:
Fix improper register assignment").
These bugs shouldn't happen with clang.

But the global named register works slightly differently in clang. For
the global, the value is just the value of the register itself,
whatever it is. Read/write from global named register is just like
direct read/write to the register

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

#1693227

On Thu, Jul 20, 2017 at 06:30:24PM +0300, Andrey Ryabinin wrote:
> FWIW bellow is my understanding of what's going on.
> 
> It seems clang treats local named register almost the same as ordinary
> local variables.
> The only difference is that before reading the register variable clang
> puts variable's value into the specified register.
> 
> So clang just assigns stack slot for the variable __sp where it's
> going to keep variable's value.
> But since __sp is unitialized (we haven't assign anything to it), the
> value of the __sp is some garbage from stack.
> inline asm specifies __sp as input, so clang assumes that it have to
> load __sp into 'rsp' because inline asm is going to use
> it. And it just loads garbage from stack into 'rsp'
> 
> In fact, such behavior (I mean storing the value on stack and loading
> into reg before the use) is very useful.
> Clang's behavior allows to keep the value assigned to the
> call-clobbered register across the function calls.
> 
> Unlike clang, gcc assigns value to the register right away and doesn't
> store the value anywhere else. So if the reg is
> call clobbered register you have to be absolutely sure that there is
> no subsequent function call that might clobber the register.
> 
> E.g. see some real examples
> https://patchwork.kernel.org/patch/4111971/ or 98d4ded60bda("msm: scm:
> Fix improper register assignment").
> These bugs shouldn't happen with clang.
> 
> But the global named register works slightly differently in clang. For
> the global, the value is just the value of the register itself,
> whatever it is. Read/write from global named register is just like
> direct read/write to the register

Thanks, that clears up a lot of the confusion for me.

Still, unfortunately, I don't think that's going to work for GCC.
Changing the '__sp' register variable to global in the header file
causes it to make a *bunch* of changes across the kernel, even in
functions which don't do inline asm.  It seems to be disabling some
optimizations across the board.

I do have another idea, which is to replace all uses of

  asm(" ... call foo ... " : outputs : inputs : clobbers);

with a new ASM_CALL macro:

  ASM_CALL(" ... call foo ... ", outputs, inputs, clobbers);

Then the compiler differences can be abstracted out, with GCC adding
"sp" as an output constraint and clang doing nothing (for now).

-- 
Josh

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

#1693511

On 07/20/2017 11:56 PM, Josh Poimboeuf wrote:
> On Thu, Jul 20, 2017 at 06:30:24PM +0300, Andrey Ryabinin wrote:
>> FWIW bellow is my understanding of what's going on.
>>
>> It seems clang treats local named register almost the same as ordinary
>> local variables.
>> The only difference is that before reading the register variable clang
>> puts variable's value into the specified register.
>>
>> So clang just assigns stack slot for the variable __sp where it's
>> going to keep variable's value.
>> But since __sp is unitialized (we haven't assign anything to it), the
>> value of the __sp is some garbage from stack.
>> inline asm specifies __sp as input, so clang assumes that it have to
>> load __sp into 'rsp' because inline asm is going to use
>> it. And it just loads garbage from stack into 'rsp'
>>
>> In fact, such behavior (I mean storing the value on stack and loading
>> into reg before the use) is very useful.
>> Clang's behavior allows to keep the value assigned to the
>> call-clobbered register across the function calls.
>>
>> Unlike clang, gcc assigns value to the register right away and doesn't
>> store the value anywhere else. So if the reg is
>> call clobbered register you have to be absolutely sure that there is
>> no subsequent function call that might clobber the register.
>>
>> E.g. see some real examples
>> https://patchwork.kernel.org/patch/4111971/ or 98d4ded60bda("msm: scm:
>> Fix improper register assignment").
>> These bugs shouldn't happen with clang.
>>
>> But the global named register works slightly differently in clang. For
>> the global, the value is just the value of the register itself,
>> whatever it is. Read/write from global named register is just like
>> direct read/write to the register
> 
> Thanks, that clears up a lot of the confusion for me.
> 
> Still, unfortunately, I don't think that's going to work for GCC.
> Changing the '__sp' register variable to global in the header file
> causes it to make a *bunch* of changes across the kernel, even in
> functions which don't do inline asm.  It seems to be disabling some
> optimizations across the board.

All I see is just bunch of reordering of independent instructions, like this:

-ffffffff81012760:      5b                      pop    %rbx
-ffffffff81012761:      31 c0                   xor    %eax,%eax
+ffffffff81012760:      31 c0                   xor    %eax,%eax
+ffffffff81012762:      5b                      pop    %rbx

-ffffffff810c29ae:      48 83 c4 28             add    $0x28,%rsp
-ffffffff810c29b2:      89 d8                   mov    %ebx,%eax
+ffffffff810c29ae:      89 d8                   mov    %ebx,%eax
+ffffffff810c29b0:      48 83 c4 28             add    $0x28,%rsp

I haven't noticed any single bad/harmful change. The size of .text remained the same. 

And btw, arm/arm64 already use global current_stack_pointer just fine.

> I do have another idea, which is to replace all uses of
> 
>   asm(" ... call foo ... " : outputs : inputs : clobbers);
> 
> with a new ASM_CALL macro:
> 
>   ASM_CALL(" ... call foo ... ", outputs, inputs, clobbers);
> 
> Then the compiler differences can be abstracted out, with GCC adding
> "sp" as an output constraint and clang doing nothing (for now).
> 

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

#1693700

On Fri, Jul 21, 2017 at 12:13:31PM +0300, Andrey Ryabinin wrote:
> > Still, unfortunately, I don't think that's going to work for GCC.
> > Changing the '__sp' register variable to global in the header file
> > causes it to make a *bunch* of changes across the kernel, even in
> > functions which don't do inline asm.  It seems to be disabling some
> > optimizations across the board.
> 
> All I see is just bunch of reordering of independent instructions, like this:
> 
> -ffffffff81012760:      5b                      pop    %rbx
> -ffffffff81012761:      31 c0                   xor    %eax,%eax
> +ffffffff81012760:      31 c0                   xor    %eax,%eax
> +ffffffff81012762:      5b                      pop    %rbx
> 
> -ffffffff810c29ae:      48 83 c4 28             add    $0x28,%rsp
> -ffffffff810c29b2:      89 d8                   mov    %ebx,%eax
> +ffffffff810c29ae:      89 d8                   mov    %ebx,%eax
> +ffffffff810c29b0:      48 83 c4 28             add    $0x28,%rsp
> 
> I haven't noticed any single bad/harmful change. The size of .text remained the same. 

I compiled with -ffunction-sections to make the comparisons easier.  The
reordering is much more extreme than your example.  (This is with GCC 7,
btw).  And it's not just reordering of instructions.  It's control flow
changes as well.

Also, the text size grew a little:

      text	   data	     bss	     dec	    hex	filename
  10630602	8295074	16461824	35387500	21bf86c	vmlinux.before
  10634013	8295074	16461824	35390911	21c05bf	vmlinux.after

A small two-line change, which is supposed to be a noop, or at least
should only affect a small number of functions, but which instead
affects optimization decisions across the entire kernel, is actively
harmful IMO.

> And btw, arm/arm64 already use global current_stack_pointer just fine.

I wonder if they looked for the impact.

-- 
Josh

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

#1699189

El Thu, Jul 20, 2017 at 03:56:52PM -0500 Josh Poimboeuf ha dit:

> On Thu, Jul 20, 2017 at 06:30:24PM +0300, Andrey Ryabinin wrote:
> > FWIW bellow is my understanding of what's going on.
> > 
> > It seems clang treats local named register almost the same as ordinary
> > local variables.
> > The only difference is that before reading the register variable clang
> > puts variable's value into the specified register.
> > 
> > So clang just assigns stack slot for the variable __sp where it's
> > going to keep variable's value.
> > But since __sp is unitialized (we haven't assign anything to it), the
> > value of the __sp is some garbage from stack.
> > inline asm specifies __sp as input, so clang assumes that it have to
> > load __sp into 'rsp' because inline asm is going to use
> > it. And it just loads garbage from stack into 'rsp'
> > 
> > In fact, such behavior (I mean storing the value on stack and loading
> > into reg before the use) is very useful.
> > Clang's behavior allows to keep the value assigned to the
> > call-clobbered register across the function calls.
> > 
> > Unlike clang, gcc assigns value to the register right away and doesn't
> > store the value anywhere else. So if the reg is
> > call clobbered register you have to be absolutely sure that there is
> > no subsequent function call that might clobber the register.
> > 
> > E.g. see some real examples
> > https://patchwork.kernel.org/patch/4111971/ or 98d4ded60bda("msm: scm:
> > Fix improper register assignment").
> > These bugs shouldn't happen with clang.
> > 
> > But the global named register works slightly differently in clang. For
> > the global, the value is just the value of the register itself,
> > whatever it is. Read/write from global named register is just like
> > direct read/write to the register
> 
> Thanks, that clears up a lot of the confusion for me.
> 
> Still, unfortunately, I don't think that's going to work for GCC.
> Changing the '__sp' register variable to global in the header file
> causes it to make a *bunch* of changes across the kernel, even in
> functions which don't do inline asm.  It seems to be disabling some
> optimizations across the board.
> 
> I do have another idea, which is to replace all uses of
> 
>   asm(" ... call foo ... " : outputs : inputs : clobbers);
> 
> with a new ASM_CALL macro:
> 
>   ASM_CALL(" ... call foo ... ", outputs, inputs, clobbers);
> 
> Then the compiler differences can be abstracted out, with GCC adding
> "sp" as an output constraint and clang doing nothing (for now).

The idea sounds interesting, however I see two issues with ASM_CALL():

Inline assembly expects the individual elements of outputs, inputs and
clobbers to be comma separated, and so does the macro for it's
parameters.

The assembler template can refer to the position of output and input
operands, adding "sp" as output changes the positions of the inputs
wrt to the clang version.

Not sure how to move forward from here. Not even using an ugly #ifdef
seems to be a halfway reasonable solution, since get_user() isn't the
only place using this construct and #ifdefs would result in highly
redundant macros in multiple places.

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

#1699194

On Fri, Jul 28, 2017 at 07:55:21PM -0500, Josh Poimboeuf wrote:
> +#define ASM_CALL(str, outputs, inputs, clobbers...) \
> +	asm volatile(str : outputs : inputs : "sp", ## clobbers)

And note this part isn't right, the sp should be in the output operands.

-- 
Josh

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

#1699196

On Fri, Jul 28, 2017 at 05:38:52PM -0700, Matthias Kaehlcke wrote:
> El Thu, Jul 20, 2017 at 03:56:52PM -0500 Josh Poimboeuf ha dit:
> 
> > On Thu, Jul 20, 2017 at 06:30:24PM +0300, Andrey Ryabinin wrote:
> > > FWIW bellow is my understanding of what's going on.
> > > 
> > > It seems clang treats local named register almost the same as ordinary
> > > local variables.
> > > The only difference is that before reading the register variable clang
> > > puts variable's value into the specified register.
> > > 
> > > So clang just assigns stack slot for the variable __sp where it's
> > > going to keep variable's value.
> > > But since __sp is unitialized (we haven't assign anything to it), the
> > > value of the __sp is some garbage from stack.
> > > inline asm specifies __sp as input, so clang assumes that it have to
> > > load __sp into 'rsp' because inline asm is going to use
> > > it. And it just loads garbage from stack into 'rsp'
> > > 
> > > In fact, such behavior (I mean storing the value on stack and loading
> > > into reg before the use) is very useful.
> > > Clang's behavior allows to keep the value assigned to the
> > > call-clobbered register across the function calls.
> > > 
> > > Unlike clang, gcc assigns value to the register right away and doesn't
> > > store the value anywhere else. So if the reg is
> > > call clobbered register you have to be absolutely sure that there is
> > > no subsequent function call that might clobber the register.
> > > 
> > > E.g. see some real examples
> > > https://patchwork.kernel.org/patch/4111971/ or 98d4ded60bda("msm: scm:
> > > Fix improper register assignment").
> > > These bugs shouldn't happen with clang.
> > > 
> > > But the global named register works slightly differently in clang. For
> > > the global, the value is just the value of the register itself,
> > > whatever it is. Read/write from global named register is just like
> > > direct read/write to the register
> > 
> > Thanks, that clears up a lot of the confusion for me.
> > 
> > Still, unfortunately, I don't think that's going to work for GCC.
> > Changing the '__sp' register variable to global in the header file
> > causes it to make a *bunch* of changes across the kernel, even in
> > functions which don't do inline asm.  It seems to be disabling some
> > optimizations across the board.
> > 
> > I do have another idea, which is to replace all uses of
> > 
> >   asm(" ... call foo ... " : outputs : inputs : clobbers);
> > 
> > with a new ASM_CALL macro:
> > 
> >   ASM_CALL(" ... call foo ... ", outputs, inputs, clobbers);
> > 
> > Then the compiler differences can be abstracted out, with GCC adding
> > "sp" as an output constraint and clang doing nothing (for now).
> 
> The idea sounds interesting, however I see two issues with ASM_CALL():
> 
> Inline assembly expects the individual elements of outputs, inputs and
> clobbers to be comma separated, and so does the macro for it's
> parameters.

I think this isn't a problem, see the (untested and unfinished) patch
below for an idea of how the arguments can be handled.

> The assembler template can refer to the position of output and input
> operands, adding "sp" as output changes the positions of the inputs
> wrt to the clang version.

True, though I think we could convert all ASM_CALL users to use named
operands instead of the (bug-prone) numbered ones.  It would be an
improvement regardless.

> Not sure how to move forward from here. Not even using an ugly #ifdef
> seems to be a halfway reasonable solution, since get_user() isn't the
> only place using this construct and #ifdefs would result in highly
> redundant macros in multiple places.

I still think ASM_CALL might work.  The below patch is what I came up
with last week before I got pulled into some other work.  I'll be out on
vacation next week but I hope to finish the patch after that.


diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index 1b020381ab38..5da4bcbeebab 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -216,14 +216,14 @@ static inline int alternatives_text_reserved(void *start, void *end)
  * Otherwise, old function is used.
  */
 #define alternative_call_2(oldfunc, newfunc1, feature1, newfunc2, feature2,   \
-			   output, input...)				      \
+			   output, input, clobbers...)			      \
 {									      \
-	register void *__sp asm(_ASM_SP);				      \
-	asm volatile (ALTERNATIVE_2("call %P[old]", "call %P[new1]", feature1,\
-		"call %P[new2]", feature2)				      \
-		: output, "+r" (__sp)					      \
-		: [old] "i" (oldfunc), [new1] "i" (newfunc1),		      \
-		  [new2] "i" (newfunc2), ## input);			      \
+	ASM_CALL(ALTERNATIVE_2("call %P[old]", "call %P[new1]", feature1,     \
+			       "call %P[new2]", feature2),		      \
+		 ARGS(output),						      \
+		 ARGS([old] "i" (oldfunc), [new1] "i" (newfunc1),	      \
+		      [new2] "i" (newfunc2), ARGS(input)),		      \
+		 ## clobbers);						      \
 }
 
 /*
diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
index b4a0d43248cf..21adcc8e739f 100644
--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -45,8 +45,8 @@ static inline void clear_page(void *page)
 			   clear_page_rep, X86_FEATURE_REP_GOOD,
 			   clear_page_erms, X86_FEATURE_ERMS,
 			   "=D" (page),
-			   "0" (page)
-			   : "memory", "rax", "rcx");
+			   "0" (page),
+			   ARGS("memory", "rax", "rcx"));
 }
 
 void copy_page(void *to, void *from);
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index cb976bab6299..2a585b799a3e 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -471,8 +471,7 @@ int paravirt_disable_iospace(void);
  */
 #ifdef CONFIG_X86_32
 #define PVOP_VCALL_ARGS							\
-	unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;	\
-	register void *__sp asm("esp")
+	unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;
 #define PVOP_CALL_ARGS			PVOP_VCALL_ARGS
 
 #define PVOP_CALL_ARG1(x)		"a" ((unsigned long)(x))
@@ -492,8 +491,7 @@ int paravirt_disable_iospace(void);
 /* [re]ax isn't an arg, but the return val */
 #define PVOP_VCALL_ARGS						\
 	unsigned long __edi = __edi, __esi = __esi,		\
-		__edx = __edx, __ecx = __ecx, __eax = __eax;	\
-	register void *__sp asm("rsp")
+		__edx = __edx, __ecx = __ecx, __eax = __eax;
 #define PVOP_CALL_ARGS		PVOP_VCALL_ARGS
 
 #define PVOP_CALL_ARG1(x)		"D" ((unsigned long)(x))
@@ -541,24 +539,24 @@ int paravirt_disable_iospace(void);
 		/* This is 32-bit specific, but is okay in 64-bit */	\
 		/* since this condition will never hold */		\
 		if (sizeof(rettype) > sizeof(unsigned long)) {		\
-			asm volatile(pre				\
-				     paravirt_alt(PARAVIRT_CALL)	\
-				     post				\
-				     : call_clbr, "+r" (__sp)		\
-				     : paravirt_type(op),		\
-				       paravirt_clobber(clbr),		\
-				       ##__VA_ARGS__			\
-				     : "memory", "cc" extra_clbr);	\
+			ASM_CALL(pre					\
+				 paravirt_alt(PARAVIRT_CALL)		\
+				 post,					\
+				 ARGS(call_clbr),			\
+				 ARGS(paravirt_type(op),		\
+				      paravirt_clobber(clbr),		\
+				      ##__VA_ARGS__),			\
+				 ARGS("memory", "cc" extra_clbr));	\
 			__ret = (rettype)((((u64)__edx) << 32) | __eax); \
 		} else {						\
-			asm volatile(pre				\
-				     paravirt_alt(PARAVIRT_CALL)	\
-				     post				\
-				     : call_clbr, "+r" (__sp)		\
-				     : paravirt_type(op),		\
-				       paravirt_clobber(clbr),		\
-				       ##__VA_ARGS__			\
-				     : "memory", "cc" extra_clbr);	\
+			ASM_CALL(pre					\
+				 paravirt_alt(PARAVIRT_CALL)		\
+				 post,					\
+				 ARGS(call_clbr),			\
+				 ARGS(paravirt_type(op),		\
+				      paravirt_clobber(clbr),		\
+				      ##__VA_ARGS__),			\
+				 ARGS("memory", "cc" extra_clbr));	\
 			__ret = (rettype)(__eax & PVOP_RETMASK(rettype));	\
 		}							\
 		__ret;							\
@@ -578,14 +576,14 @@ int paravirt_disable_iospace(void);
 	({								\
 		PVOP_VCALL_ARGS;					\
 		PVOP_TEST_NULL(op);					\
-		asm volatile(pre					\
-			     paravirt_alt(PARAVIRT_CALL)		\
-			     post					\
-			     : call_clbr, "+r" (__sp)			\
-			     : paravirt_type(op),			\
-			       paravirt_clobber(clbr),			\
-			       ##__VA_ARGS__				\
-			     : "memory", "cc" extra_clbr);		\
+		ASM_CALL(pre						\
+			 paravirt_alt(PARAVIRT_CALL)			\
+			 post,						\
+			 ARGS(call_clbr),				\
+			 ARGS(paravirt_type(op),			\
+			      paravirt_clobber(clbr),			\
+			      ##__VA_ARGS__),				\
+			 ARGS("memory", "cc" extra_clbr));		\
 	})
 
 #define __PVOP_VCALL(op, pre, post, ...)				\
diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
index ec1f3c651150..19a034cbde37 100644
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -102,16 +102,14 @@ static __always_inline bool should_resched(int preempt_offset)
   extern asmlinkage void ___preempt_schedule(void);
 # define __preempt_schedule()					\
 ({								\
-	register void *__sp asm(_ASM_SP);			\
-	asm volatile ("call ___preempt_schedule" : "+r"(__sp));	\
+	ASM_CALL("call ___preempt_schedule",,);			\
 })
 
   extern asmlinkage void preempt_schedule(void);
   extern asmlinkage void ___preempt_schedule_notrace(void);
 # define __preempt_schedule_notrace()					\
 ({									\
-	register void *__sp asm(_ASM_SP);				\
-	asm volatile ("call ___preempt_schedule_notrace" : "+r"(__sp));	\
+	ASM_CALL("call ___preempt_schedule_notrace",,);			\
 })
   extern asmlinkage void preempt_schedule_notrace(void);
 #endif
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 550bc4ab0df4..2bbfb777c8bb 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -677,20 +677,19 @@ static inline void sync_core(void)
 	 * Like all of Linux's memory ordering operations, this is a
 	 * compiler barrier as well.
 	 */
-	register void *__sp asm(_ASM_SP);
 
 #ifdef CONFIG_X86_32
-	asm volatile (
+	ASM_CALL(
 		"pushfl\n\t"
 		"pushl %%cs\n\t"
 		"pushl $1f\n\t"
 		"iret\n\t"
-		"1:"
-		: "+r" (__sp) : : "memory");
+		"1:",
+		, , "memory");
 #else
 	unsigned int tmp;
 
-	asm volatile (
+	ASM_CALL(
 		UNWIND_HINT_SAVE
 		"mov %%ss, %0\n\t"
 		"pushq %q0\n\t"
@@ -702,8 +701,8 @@ static inline void sync_core(void)
 		"pushq $1f\n\t"
 		"iretq\n\t"
 		UNWIND_HINT_RESTORE
-		"1:"
-		: "=&r" (tmp), "+r" (__sp) : : "cc", "memory");
+		"1:",
+		"=&r" (tmp), , ARGS("cc", "memory"));
 #endif
 }
 
diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
index a34e0d4b957d..9d20f4fb5d7c 100644
--- a/arch/x86/include/asm/rwsem.h
+++ b/arch/x86/include/asm/rwsem.h
@@ -99,25 +99,24 @@ static inline bool __down_read_trylock(struct rw_semaphore *sem)
 /*
  * lock for writing
  */
-#define ____down_write(sem, slow_path)			\
-({							\
-	long tmp;					\
-	struct rw_semaphore* ret;			\
-	register void *__sp asm(_ASM_SP);		\
-							\
-	asm volatile("# beginning down_write\n\t"	\
-		     LOCK_PREFIX "  xadd      %1,(%4)\n\t"	\
-		     /* adds 0xffff0001, returns the old value */ \
-		     "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t" \
-		     /* was the active mask 0 before? */\
-		     "  jz        1f\n"			\
-		     "  call " slow_path "\n"		\
-		     "1:\n"				\
-		     "# ending down_write"		\
-		     : "+m" (sem->count), "=d" (tmp), "=a" (ret), "+r" (__sp) \
-		     : "a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS) \
-		     : "memory", "cc");			\
-	ret;						\
+#define ____down_write(sem, slow_path)					\
+({									\
+	long tmp;							\
+	struct rw_semaphore* ret;					\
+									\
+	ASM_CALL("# beginning down_write\n\t"				\
+		 LOCK_PREFIX "  xadd      %1,(%3)\n\t"			\
+		 /* adds 0xffff0001, returns the old value */		\
+		 "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t" \
+		 /* was the active mask 0 before? */			\
+		 "  jz        1f\n"					\
+		 "  call " slow_path "\n"				\
+		 "1:\n"							\
+		 "# ending down_write",					\
+		 ARGS("+m" (sem->count), "=d" (tmp), "=a" (ret)),	\
+		 ARGS("a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS)),	\
+		 ARGS("memory", "cc"));					\
+	ret;								\
 })
 
 static inline void __down_write(struct rw_semaphore *sem)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 184eb9894dae..c7be076ee703 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -166,12 +166,11 @@ __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_CALL("call __get_user_%P3",					\
+		 ARGS("=a" (__ret_gu), "=r" (__val_gu)),		\
+		 ARGS("0" (ptr), "i" (sizeof(*(ptr)))));		\
 	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
 	__builtin_expect(__ret_gu, 0);					\
 })
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index b16f6a1d8b26..8b63e2cb1eaf 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -38,10 +38,9 @@ copy_user_generic(void *to, const void *from, unsigned len)
 			 X86_FEATURE_REP_GOOD,
 			 copy_user_enhanced_fast_string,
 			 X86_FEATURE_ERMS,
-			 ASM_OUTPUT2("=a" (ret), "=D" (to), "=S" (from),
-				     "=d" (len)),
-			 "1" (to), "2" (from), "3" (len)
-			 : "memory", "rcx", "r8", "r9", "r10", "r11");
+			 ARGS("=a" (ret), "=D" (to), "=S" (from), "=d" (len)),
+			 ARGS("1" (to), "2" (from), "3" (len)),
+			 ARGS("memory", "rcx", "r8", "r9", "r10", "r11"));
 	return ret;
 }
 
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index 11071fcd630e..2feddeeec1d1 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -114,9 +114,8 @@ extern struct { char _entry[32]; } hypercall_page[];
 	register unsigned long __arg3 asm(__HYPERCALL_ARG3REG) = __arg3; \
 	register unsigned long __arg4 asm(__HYPERCALL_ARG4REG) = __arg4; \
 	register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5; \
-	register void *__sp asm(_ASM_SP);
 
-#define __HYPERCALL_0PARAM	"=r" (__res), "+r" (__sp)
+#define __HYPERCALL_0PARAM	"=r" (__res)
 #define __HYPERCALL_1PARAM	__HYPERCALL_0PARAM, "+r" (__arg1)
 #define __HYPERCALL_2PARAM	__HYPERCALL_1PARAM, "+r" (__arg2)
 #define __HYPERCALL_3PARAM	__HYPERCALL_2PARAM, "+r" (__arg3)
@@ -146,10 +145,10 @@ extern struct { char _entry[32]; } hypercall_page[];
 ({									\
 	__HYPERCALL_DECLS;						\
 	__HYPERCALL_0ARG();						\
-	asm volatile (__HYPERCALL					\
-		      : __HYPERCALL_0PARAM				\
-		      : __HYPERCALL_ENTRY(name)				\
-		      : __HYPERCALL_CLOBBER0);				\
+	ASM_CALL(__HYPERCALL,						\
+		 __HYPERCALL_0PARAM,					\
+		 __HYPERCALL_ENTRY(name),				\
+		 __HYPERCALL_CLOBBER0);					\
 	(type)__res;							\
 })
 
@@ -157,10 +156,10 @@ extern struct { char _entry[32]; } hypercall_page[];
 ({									\
 	__HYPERCALL_DECLS;						\
 	__HYPERCALL_1ARG(a1);						\
-	asm volatile (__HYPERCALL					\
-		      : __HYPERCALL_1PARAM				\
-		      : __HYPERCALL_ENTRY(name)				\
-		      : __HYPERCALL_CLOBBER1);				\
+	ASM_CALL(__HYPERCALL,						\
+		 __HYPERCALL_1PARAM,					\
+		 __HYPERCALL_ENTRY(name),				\
+		 __HYPERCALL_CLOBBER1);					\
 	(type)__res;							\
 })
 
@@ -168,10 +167,10 @@ extern struct { char _entry[32]; } hypercall_page[];
 ({									\
 	__HYPERCALL_DECLS;						\
 	__HYPERCALL_2ARG(a1, a2);					\
-	asm volatile (__HYPERCALL					\
-		      : __HYPERCALL_2PARAM				\
-		      : __HYPERCALL_ENTRY(name)				\
-		      : __HYPERCALL_CLOBBER2);				\
+	ASM_CALL(__HYPERCALL,						\
+		 __HYPERCALL_2PARAM,					\
+		 __HYPERCALL_ENTRY(name),				\
+		 __HYPERCALL_CLOBBER2);					\
 	(type)__res;							\
 })
 
@@ -179,10 +178,10 @@ extern struct { char _entry[32]; } hypercall_page[];
 ({									\
 	__HYPERCALL_DECLS;						\
 	__HYPERCALL_3ARG(a1, a2, a3);					\
-	asm volatile (__HYPERCALL					\
-		      : __HYPERCALL_3PARAM				\
-		      : __HYPERCALL_ENTRY(name)				\
-		      : __HYPERCALL_CLOBBER3);				\
+	ASM_CALL(__HYPERCALL,						\
+		 __HYPERCALL_3PARAM,					\
+		 __HYPERCALL_ENTRY(name),				\
+		 __HYPERCALL_CLOBBER3);					\
 	(type)__res;							\
 })
 
@@ -190,10 +189,10 @@ extern struct { char _entry[32]; } hypercall_page[];
 ({									\
 	__HYPERCALL_DECLS;						\
 	__HYPERCALL_4ARG(a1, a2, a3, a4);				\
-	asm volatile (__HYPERCALL					\
-		      : __HYPERCALL_4PARAM				\
-		      : __HYPERCALL_ENTRY(name)				\
-		      : __HYPERCALL_CLOBBER4);				\
+	ASM_CALL(__HYPERCALL,						\
+		 __HYPERCALL_4PARAM,					\
+		 __HYPERCALL_ENTRY(name),				\
+		 __HYPERCALL_CLOBBER4);					\
 	(type)__res;							\
 })
 
@@ -201,10 +200,10 @@ extern struct { char _entry[32]; } hypercall_page[];
 ({									\
 	__HYPERCALL_DECLS;						\
 	__HYPERCALL_5ARG(a1, a2, a3, a4, a5);				\
-	asm volatile (__HYPERCALL					\
-		      : __HYPERCALL_5PARAM				\
-		      : __HYPERCALL_ENTRY(name)				\
-		      : __HYPERCALL_CLOBBER5);				\
+	ASM_CALL(__HYPERCALL,						\
+		 __HYPERCALL_5PARAM,					\
+		 __HYPERCALL_ENTRY(name),				\
+		 __HYPERCALL_CLOBBER5);					\
 	(type)__res;							\
 })
 
@@ -218,10 +217,10 @@ privcmd_call(unsigned call,
 	__HYPERCALL_5ARG(a1, a2, a3, a4, a5);
 
 	stac();
-	asm volatile("call *%[call]"
-		     : __HYPERCALL_5PARAM
-		     : [call] "a" (&hypercall_page[call])
-		     : __HYPERCALL_CLOBBER5);
+	ASM_CALL("call *%[call]",
+		 __HYPERCALL_5PARAM,
+		[call] "a" (&hypercall_page[call]),
+		__HYPERCALL_CLOBBER5);
 	clac();
 
 	return (long)__res;
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index fb0055953fbc..4d0d326029a7 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5284,16 +5284,15 @@ static void fetch_possible_mmx_operand(struct x86_emulate_ctxt *ctxt,
 
 static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *))
 {
-	register void *__sp asm(_ASM_SP);
 	ulong flags = (ctxt->eflags & EFLAGS_MASK) | X86_EFLAGS_IF;
 
 	if (!(ctxt->d & ByteOp))
 		fop += __ffs(ctxt->dst.bytes) * FASTOP_SIZE;
 
-	asm("push %[flags]; popf; call *%[fastop]; pushf; pop %[flags]\n"
-	    : "+a"(ctxt->dst.val), "+d"(ctxt->src.val), [flags]"+D"(flags),
-	      [fastop]"+S"(fop), "+r"(__sp)
-	    : "c"(ctxt->src2.val));
+	ASM_CALL("push %[flags]; popf; call *%[fastop]; pushf; pop %[flags]\n",
+		 ARGS("+a"(ctxt->dst.val), "+d"(ctxt->src.val),
+		      [flags]"+D"(flags), [fastop]"+S"(fop)),
+		 ARGS("c"(ctxt->src2.val)));
 
 	ctxt->eflags = (ctxt->eflags & ~EFLAGS_MASK) | (flags & EFLAGS_MASK);
 	if (!fop) /* exception is returned in fop variable */
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ffd469ecad57..d42806ad3c9c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8671,7 +8671,6 @@ static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
 static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
 {
 	u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
-	register void *__sp asm(_ASM_SP);
 
 	if ((exit_intr_info & (INTR_INFO_VALID_MASK | INTR_INFO_INTR_TYPE_MASK))
 			== (INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR)) {
@@ -8686,7 +8685,7 @@ static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
 		vector =  exit_intr_info & INTR_INFO_VECTOR_MASK;
 		desc = (gate_desc *)vmx->host_idt_base + vector;
 		entry = gate_offset(*desc);
-		asm volatile(
+		ASM_CALL(
 #ifdef CONFIG_X86_64
 			"mov %%" _ASM_SP ", %[sp]\n\t"
 			"and $0xfffffffffffffff0, %%" _ASM_SP "\n\t"
@@ -8695,16 +8694,14 @@ static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
 #endif
 			"pushf\n\t"
 			__ASM_SIZE(push) " $%c[cs]\n\t"
-			"call *%[entry]\n\t"
-			:
+			"call *%[entry]\n\t",
 #ifdef CONFIG_X86_64
-			[sp]"=&r"(tmp),
+			[sp]"=&r"(tmp)
 #endif
-			"+r"(__sp)
-			:
-			[entry]"r"(entry),
-			[ss]"i"(__KERNEL_DS),
-			[cs]"i"(__KERNEL_CS)
+			,
+			ARGS([entry]"r"(entry),
+			     [ss]"i"(__KERNEL_DS),
+			     [cs]"i"(__KERNEL_CS))
 			);
 	}
 }
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 2a1fa10c6a98..ca642ac3a390 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -802,7 +802,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	if (is_vmalloc_addr((void *)address) &&
 	    (((unsigned long)tsk->stack - 1 - address < PAGE_SIZE) ||
 	     address - ((unsigned long)tsk->stack + THREAD_SIZE) < PAGE_SIZE)) {
-		register void *__sp asm("rsp");
 		unsigned long stack = this_cpu_read(orig_ist.ist[DOUBLEFAULT_STACK]) - sizeof(void *);
 		/*
 		 * We're likely to be running with very little stack space
@@ -814,13 +813,12 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 		 * and then double-fault, though, because we're likely to
 		 * break the console driver and lose most of the stack dump.
 		 */
-		asm volatile ("movq %[stack], %%rsp\n\t"
-			      "call handle_stack_overflow\n\t"
-			      "1: jmp 1b"
-			      : "+r" (__sp)
-			      : "D" ("kernel stack overflow (page fault)"),
-				"S" (regs), "d" (address),
-				[stack] "rm" (stack));
+		ASM_CALL("movq %[stack], %%rsp\n\t"
+			 "call handle_stack_overflow\n\t"
+			 "1: jmp 1b",
+			 ,
+			 ARGS("D" ("kernel stack overflow (page fault)"),
+			      "S" (regs), "d" (address), [stack] "rm" (stack)));
 		unreachable();
 	}
 #endif
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 3f8c88e29a46..3a57f32a0bfd 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -589,4 +589,18 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 	(_________p1); \
 })
 
+#define ARGS(args...) args
+
+#ifdef CONFIG_FRAME_POINTER
+
+#define ASM_CALL(str, outputs, inputs, clobbers...) \
+	asm volatile(str : outputs : inputs : "sp", ## clobbers)
+
+#else
+
+#define ASM_CALL(str, outputs, inputs, clobbers...) \
+	asm volatile(str : outputs : inputs : clobbers);
+
+#endif
+
 #endif /* __LINUX_COMPILER_H */
diff --git a/tools/objtool/Documentation/stack-validation.txt b/tools/objtool/Documentation/stack-validation.txt
index 6a1af43862df..766a00ebf80c 100644
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -193,13 +193,8 @@ they mean, and suggestions for how to fix them.
 
    If it's a GCC-compiled .c file, the error may be because the function
    uses an inline asm() statement which has a "call" instruction.  An
-   asm() statement with a call instruction must declare the use of the
-   stack pointer in its output operand.  For example, on x86_64:
-
-     register void *__sp asm("rsp");
-     asm volatile("call func" : "+r" (__sp));
-
-   Otherwise the stack frame may not get created before the call.
+   asm() statement with a call instruction must use the ASM_CALL macro,
+   which forces the frame pointer to be saved before the call.
 
 
 2. file.o: warning: objtool: .text+0x53: unreachable instruction
@@ -221,7 +216,7 @@ they mean, and suggestions for how to fix them.
    change ENDPROC to END.
 
 
-4. file.o: warning: objtool: func(): can't find starting instruction
+3. file.o: warning: objtool: func(): can't find starting instruction
    or
    file.o: warning: objtool: func()+0x11dd: can't decode instruction
 
@@ -230,7 +225,7 @@ they mean, and suggestions for how to fix them.
    section like .data or .rodata.
 
 
-5. file.o: warning: objtool: func()+0x6: unsupported instruction in callable function
+4. file.o: warning: objtool: func()+0x6: unsupported instruction in callable function
 
    This is a kernel entry/exit instruction like sysenter or iret.  Such
    instructions aren't allowed in a callable function, and are most
@@ -239,7 +234,7 @@ they mean, and suggestions for how to fix them.
    annotated with the unwind hint macros in asm/unwind_hints.h.
 
 
-6. file.o: warning: objtool: func()+0x26: sibling call from callable instruction with modified stack frame
+5. file.o: warning: objtool: func()+0x26: sibling call from callable instruction with modified stack frame
 
    This is a dynamic jump or a jump to an undefined symbol.  Objtool
    assumed it's a sibling call and detected that the frame pointer
@@ -253,7 +248,7 @@ they mean, and suggestions for how to fix them.
    the unwind hint macros in asm/unwind_hints.h.
 
 
-7. file: warning: objtool: func()+0x5c: stack state mismatch
+6. file: warning: objtool: func()+0x5c: stack state mismatch
 
    The instruction's frame pointer state is inconsistent, depending on
    which execution path was taken to reach the instruction.
@@ -270,7 +265,7 @@ they mean, and suggestions for how to fix them.
    asm/unwind_hints.h.
 
 
-8. file.o: warning: objtool: funcA() falls through to next function funcB()
+7. file.o: warning: objtool: funcA() falls through to next function funcB()
 
    This means that funcA() doesn't end with a return instruction or an
    unconditional jump, and that objtool has determined that the function

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

#1699197

El Fri, Jul 28, 2017 at 07:55:21PM -0500 Josh Poimboeuf ha dit:

> On Fri, Jul 28, 2017 at 05:38:52PM -0700, Matthias Kaehlcke wrote:
> > El Thu, Jul 20, 2017 at 03:56:52PM -0500 Josh Poimboeuf ha dit:
> > 
> > > On Thu, Jul 20, 2017 at 06:30:24PM +0300, Andrey Ryabinin wrote:
> > > > FWIW bellow is my understanding of what's going on.
> > > > 
> > > > It seems clang treats local named register almost the same as ordinary
> > > > local variables.
> > > > The only difference is that before reading the register variable clang
> > > > puts variable's value into the specified register.
> > > > 
> > > > So clang just assigns stack slot for the variable __sp where it's
> > > > going to keep variable's value.
> > > > But since __sp is unitialized (we haven't assign anything to it), the
> > > > value of the __sp is some garbage from stack.
> > > > inline asm specifies __sp as input, so clang assumes that it have to
> > > > load __sp into 'rsp' because inline asm is going to use
> > > > it. And it just loads garbage from stack into 'rsp'
> > > > 
> > > > In fact, such behavior (I mean storing the value on stack and loading
> > > > into reg before the use) is very useful.
> > > > Clang's behavior allows to keep the value assigned to the
> > > > call-clobbered register across the function calls.
> > > > 
> > > > Unlike clang, gcc assigns value to the register right away and doesn't
> > > > store the value anywhere else. So if the reg is
> > > > call clobbered register you have to be absolutely sure that there is
> > > > no subsequent function call that might clobber the register.
> > > > 
> > > > E.g. see some real examples
> > > > https://patchwork.kernel.org/patch/4111971/ or 98d4ded60bda("msm: scm:
> > > > Fix improper register assignment").
> > > > These bugs shouldn't happen with clang.
> > > > 
> > > > But the global named register works slightly differently in clang. For
> > > > the global, the value is just the value of the register itself,
> > > > whatever it is. Read/write from global named register is just like
> > > > direct read/write to the register
> > > 
> > > Thanks, that clears up a lot of the confusion for me.
> > > 
> > > Still, unfortunately, I don't think that's going to work for GCC.
> > > Changing the '__sp' register variable to global in the header file
> > > causes it to make a *bunch* of changes across the kernel, even in
> > > functions which don't do inline asm.  It seems to be disabling some
> > > optimizations across the board.
> > > 
> > > I do have another idea, which is to replace all uses of
> > > 
> > >   asm(" ... call foo ... " : outputs : inputs : clobbers);
> > > 
> > > with a new ASM_CALL macro:
> > > 
> > >   ASM_CALL(" ... call foo ... ", outputs, inputs, clobbers);
> > > 
> > > Then the compiler differences can be abstracted out, with GCC adding
> > > "sp" as an output constraint and clang doing nothing (for now).
> > 
> > The idea sounds interesting, however I see two issues with ASM_CALL():
> > 
> > Inline assembly expects the individual elements of outputs, inputs and
> > clobbers to be comma separated, and so does the macro for it's
> > parameters.
> 
> I think this isn't a problem, see the (untested and unfinished) patch
> below for an idea of how the arguments can be handled.

Good point!

> > The assembler template can refer to the position of output and input
> > operands, adding "sp" as output changes the positions of the inputs
> > wrt to the clang version.
> 
> True, though I think we could convert all ASM_CALL users to use named
> operands instead of the (bug-prone) numbered ones.  It would be an
> improvement regardless.

Agreed, that sounds like a good solution and a desirable improvement.

> > Not sure how to move forward from here. Not even using an ugly #ifdef
> > seems to be a halfway reasonable solution, since get_user() isn't the
> > only place using this construct and #ifdefs would result in highly
> > redundant macros in multiple places.
> 
> I still think ASM_CALL might work.  The below patch is what I came up
> with last week before I got pulled into some other work.  I'll be out on
> vacation next week but I hope to finish the patch after that.

Thanks for working on this.

Enjoy your vacation!

Matthias

> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
> index 1b020381ab38..5da4bcbeebab 100644
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -216,14 +216,14 @@ static inline int alternatives_text_reserved(void *start, void *end)
>   * Otherwise, old function is used.
>   */
>  #define alternative_call_2(oldfunc, newfunc1, feature1, newfunc2, feature2,   \
> -			   output, input...)				      \
> +			   output, input, clobbers...)			      \
>  {									      \
> -	register void *__sp asm(_ASM_SP);				      \
> -	asm volatile (ALTERNATIVE_2("call %P[old]", "call %P[new1]", feature1,\
> -		"call %P[new2]", feature2)				      \
> -		: output, "+r" (__sp)					      \
> -		: [old] "i" (oldfunc), [new1] "i" (newfunc1),		      \
> -		  [new2] "i" (newfunc2), ## input);			      \
> +	ASM_CALL(ALTERNATIVE_2("call %P[old]", "call %P[new1]", feature1,     \
> +			       "call %P[new2]", feature2),		      \
> +		 ARGS(output),						      \
> +		 ARGS([old] "i" (oldfunc), [new1] "i" (newfunc1),	      \
> +		      [new2] "i" (newfunc2), ARGS(input)),		      \
> +		 ## clobbers);						      \
>  }
>  
>  /*
> diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
> index b4a0d43248cf..21adcc8e739f 100644
> --- a/arch/x86/include/asm/page_64.h
> +++ b/arch/x86/include/asm/page_64.h
> @@ -45,8 +45,8 @@ static inline void clear_page(void *page)
>  			   clear_page_rep, X86_FEATURE_REP_GOOD,
>  			   clear_page_erms, X86_FEATURE_ERMS,
>  			   "=D" (page),
> -			   "0" (page)
> -			   : "memory", "rax", "rcx");
> +			   "0" (page),
> +			   ARGS("memory", "rax", "rcx"));
>  }
>  
>  void copy_page(void *to, void *from);
> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
> index cb976bab6299..2a585b799a3e 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h
> @@ -471,8 +471,7 @@ int paravirt_disable_iospace(void);
>   */
>  #ifdef CONFIG_X86_32
>  #define PVOP_VCALL_ARGS							\
> -	unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;	\
> -	register void *__sp asm("esp")
> +	unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;
>  #define PVOP_CALL_ARGS			PVOP_VCALL_ARGS
>  
>  #define PVOP_CALL_ARG1(x)		"a" ((unsigned long)(x))
> @@ -492,8 +491,7 @@ int paravirt_disable_iospace(void);
>  /* [re]ax isn't an arg, but the return val */
>  #define PVOP_VCALL_ARGS						\
>  	unsigned long __edi = __edi, __esi = __esi,		\
> -		__edx = __edx, __ecx = __ecx, __eax = __eax;	\
> -	register void *__sp asm("rsp")
> +		__edx = __edx, __ecx = __ecx, __eax = __eax;
>  #define PVOP_CALL_ARGS		PVOP_VCALL_ARGS
>  
>  #define PVOP_CALL_ARG1(x)		"D" ((unsigned long)(x))
> @@ -541,24 +539,24 @@ int paravirt_disable_iospace(void);
>  		/* This is 32-bit specific, but is okay in 64-bit */	\
>  		/* since this condition will never hold */		\
>  		if (sizeof(rettype) > sizeof(unsigned long)) {		\
> -			asm volatile(pre				\
> -				     paravirt_alt(PARAVIRT_CALL)	\
> -				     post				\
> -				     : call_clbr, "+r" (__sp)		\
> -				     : paravirt_type(op),		\
> -				       paravirt_clobber(clbr),		\
> -				       ##__VA_ARGS__			\
> -				     : "memory", "cc" extra_clbr);	\
> +			ASM_CALL(pre					\
> +				 paravirt_alt(PARAVIRT_CALL)		\
> +				 post,					\
> +				 ARGS(call_clbr),			\
> +				 ARGS(paravirt_type(op),		\
> +				      paravirt_clobber(clbr),		\
> +				      ##__VA_ARGS__),			\
> +				 ARGS("memory", "cc" extra_clbr));	\
>  			__ret = (rettype)((((u64)__edx) << 32) | __eax); \
>  		} else {						\
> -			asm volatile(pre				\
> -				     paravirt_alt(PARAVIRT_CALL)	\
> -				     post				\
> -				     : call_clbr, "+r" (__sp)		\
> -				     : paravirt_type(op),		\
> -				       paravirt_clobber(clbr),		\
> -				       ##__VA_ARGS__			\
> -				     : "memory", "cc" extra_clbr);	\
> +			ASM_CALL(pre					\
> +				 paravirt_alt(PARAVIRT_CALL)		\
> +				 post,					\
> +				 ARGS(call_clbr),			\
> +				 ARGS(paravirt_type(op),		\
> +				      paravirt_clobber(clbr),		\
> +				      ##__VA_ARGS__),			\
> +				 ARGS("memory", "cc" extra_clbr));	\
>  			__ret = (rettype)(__eax & PVOP_RETMASK(rettype));	\
>  		}							\
>  		__ret;							\
> @@ -578,14 +576,14 @@ int paravirt_disable_iospace(void);
>  	({								\
>  		PVOP_VCALL_ARGS;					\
>  		PVOP_TEST_NULL(op);					\
> -		asm volatile(pre					\
> -			     paravirt_alt(PARAVIRT_CALL)		\
> -			     post					\
> -			     : call_clbr, "+r" (__sp)			\
> -			     : paravirt_type(op),			\
> -			       paravirt_clobber(clbr),			\
> -			       ##__VA_ARGS__				\
> -			     : "memory", "cc" extra_clbr);		\
> +		ASM_CALL(pre						\
> +			 paravirt_alt(PARAVIRT_CALL)			\
> +			 post,						\
> +			 ARGS(call_clbr),				\
> +			 ARGS(paravirt_type(op),			\
> +			      paravirt_clobber(clbr),			\
> +			      ##__VA_ARGS__),				\
> +			 ARGS("memory", "cc" extra_clbr));		\
>  	})
>  
>  #define __PVOP_VCALL(op, pre, post, ...)				\
> diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
> index ec1f3c651150..19a034cbde37 100644
> --- a/arch/x86/include/asm/preempt.h
> +++ b/arch/x86/include/asm/preempt.h
> @@ -102,16 +102,14 @@ static __always_inline bool should_resched(int preempt_offset)
>    extern asmlinkage void ___preempt_schedule(void);
>  # define __preempt_schedule()					\
>  ({								\
> -	register void *__sp asm(_ASM_SP);			\
> -	asm volatile ("call ___preempt_schedule" : "+r"(__sp));	\
> +	ASM_CALL("call ___preempt_schedule",,);			\
>  })
>  
>    extern asmlinkage void preempt_schedule(void);
>    extern asmlinkage void ___preempt_schedule_notrace(void);
>  # define __preempt_schedule_notrace()					\
>  ({									\
> -	register void *__sp asm(_ASM_SP);				\
> -	asm volatile ("call ___preempt_schedule_notrace" : "+r"(__sp));	\
> +	ASM_CALL("call ___preempt_schedule_notrace",,);			\
>  })
>    extern asmlinkage void preempt_schedule_notrace(void);
>  #endif
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 550bc4ab0df4..2bbfb777c8bb 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -677,20 +677,19 @@ static inline void sync_core(void)
>  	 * Like all of Linux's memory ordering operations, this is a
>  	 * compiler barrier as well.
>  	 */
> -	register void *__sp asm(_ASM_SP);
>  
>  #ifdef CONFIG_X86_32
> -	asm volatile (
> +	ASM_CALL(
>  		"pushfl\n\t"
>  		"pushl %%cs\n\t"
>  		"pushl $1f\n\t"
>  		"iret\n\t"
> -		"1:"
> -		: "+r" (__sp) : : "memory");
> +		"1:",
> +		, , "memory");
>  #else
>  	unsigned int tmp;
>  
> -	asm volatile (
> +	ASM_CALL(
>  		UNWIND_HINT_SAVE
>  		"mov %%ss, %0\n\t"
>  		"pushq %q0\n\t"
> @@ -702,8 +701,8 @@ static inline void sync_core(void)
>  		"pushq $1f\n\t"
>  		"iretq\n\t"
>  		UNWIND_HINT_RESTORE
> -		"1:"
> -		: "=&r" (tmp), "+r" (__sp) : : "cc", "memory");
> +		"1:",
> +		"=&r" (tmp), , ARGS("cc", "memory"));
>  #endif
>  }
>  
> diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
> index a34e0d4b957d..9d20f4fb5d7c 100644
> --- a/arch/x86/include/asm/rwsem.h
> +++ b/arch/x86/include/asm/rwsem.h
> @@ -99,25 +99,24 @@ static inline bool __down_read_trylock(struct rw_semaphore *sem)
>  /*
>   * lock for writing
>   */
> -#define ____down_write(sem, slow_path)			\
> -({							\
> -	long tmp;					\
> -	struct rw_semaphore* ret;			\
> -	register void *__sp asm(_ASM_SP);		\
> -							\
> -	asm volatile("# beginning down_write\n\t"	\
> -		     LOCK_PREFIX "  xadd      %1,(%4)\n\t"	\
> -		     /* adds 0xffff0001, returns the old value */ \
> -		     "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t" \
> -		     /* was the active mask 0 before? */\
> -		     "  jz        1f\n"			\
> -		     "  call " slow_path "\n"		\
> -		     "1:\n"				\
> -		     "# ending down_write"		\
> -		     : "+m" (sem->count), "=d" (tmp), "=a" (ret), "+r" (__sp) \
> -		     : "a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS) \
> -		     : "memory", "cc");			\
> -	ret;						\
> +#define ____down_write(sem, slow_path)					\
> +({									\
> +	long tmp;							\
> +	struct rw_semaphore* ret;					\
> +									\
> +	ASM_CALL("# beginning down_write\n\t"				\
> +		 LOCK_PREFIX "  xadd      %1,(%3)\n\t"			\
> +		 /* adds 0xffff0001, returns the old value */		\
> +		 "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t" \
> +		 /* was the active mask 0 before? */			\
> +		 "  jz        1f\n"					\
> +		 "  call " slow_path "\n"				\
> +		 "1:\n"							\
> +		 "# ending down_write",					\
> +		 ARGS("+m" (sem->count), "=d" (tmp), "=a" (ret)),	\
> +		 ARGS("a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS)),	\
> +		 ARGS("memory", "cc"));					\
> +	ret;								\
>  })
>  
>  static inline void __down_write(struct rw_semaphore *sem)
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 184eb9894dae..c7be076ee703 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -166,12 +166,11 @@ __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_CALL("call __get_user_%P3",					\
> +		 ARGS("=a" (__ret_gu), "=r" (__val_gu)),		\
> +		 ARGS("0" (ptr), "i" (sizeof(*(ptr)))));		\
>  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
>  	__builtin_expect(__ret_gu, 0);					\
>  })
> diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> index b16f6a1d8b26..8b63e2cb1eaf 100644
> --- a/arch/x86/include/asm/uaccess_64.h
> +++ b/arch/x86/include/asm/uaccess_64.h
> @@ -38,10 +38,9 @@ copy_user_generic(void *to, const void *from, unsigned len)
>  			 X86_FEATURE_REP_GOOD,
>  			 copy_user_enhanced_fast_string,
>  			 X86_FEATURE_ERMS,
> -			 ASM_OUTPUT2("=a" (ret), "=D" (to), "=S" (from),
> -				     "=d" (len)),
> -			 "1" (to), "2" (from), "3" (len)
> -			 : "memory", "rcx", "r8", "r9", "r10", "r11");
> +			 ARGS("=a" (ret), "=D" (to), "=S" (from), "=d" (len)),
> +			 ARGS("1" (to), "2" (from), "3" (len)),
> +			 ARGS("memory", "rcx", "r8", "r9", "r10", "r11"));
>  	return ret;
>  }
>  
> diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
> index 11071fcd630e..2feddeeec1d1 100644
> --- a/arch/x86/include/asm/xen/hypercall.h
> +++ b/arch/x86/include/asm/xen/hypercall.h
> @@ -114,9 +114,8 @@ extern struct { char _entry[32]; } hypercall_page[];
>  	register unsigned long __arg3 asm(__HYPERCALL_ARG3REG) = __arg3; \
>  	register unsigned long __arg4 asm(__HYPERCALL_ARG4REG) = __arg4; \
>  	register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5; \
> -	register void *__sp asm(_ASM_SP);
>  
> -#define __HYPERCALL_0PARAM	"=r" (__res), "+r" (__sp)
> +#define __HYPERCALL_0PARAM	"=r" (__res)
>  #define __HYPERCALL_1PARAM	__HYPERCALL_0PARAM, "+r" (__arg1)
>  #define __HYPERCALL_2PARAM	__HYPERCALL_1PARAM, "+r" (__arg2)
>  #define __HYPERCALL_3PARAM	__HYPERCALL_2PARAM, "+r" (__arg3)
> @@ -146,10 +145,10 @@ extern struct { char _entry[32]; } hypercall_page[];
>  ({									\
>  	__HYPERCALL_DECLS;						\
>  	__HYPERCALL_0ARG();						\
> -	asm volatile (__HYPERCALL					\
> -		      : __HYPERCALL_0PARAM				\
> -		      : __HYPERCALL_ENTRY(name)				\
> -		      : __HYPERCALL_CLOBBER0);				\
> +	ASM_CALL(__HYPERCALL,						\
> +		 __HYPERCALL_0PARAM,					\
> +		 __HYPERCALL_ENTRY(name),				\
> +		 __HYPERCALL_CLOBBER0);					\
>  	(type)__res;							\
>  })
>  
> @@ -157,10 +156,10 @@ extern struct { char _entry[32]; } hypercall_page[];
>  ({									\
>  	__HYPERCALL_DECLS;						\
>  	__HYPERCALL_1ARG(a1);						\
> -	asm volatile (__HYPERCALL					\
> -		      : __HYPERCALL_1PARAM				\
> -		      : __HYPERCALL_ENTRY(name)				\
> -		      : __HYPERCALL_CLOBBER1);				\
> +	ASM_CALL(__HYPERCALL,						\
> +		 __HYPERCALL_1PARAM,					\
> +		 __HYPERCALL_ENTRY(name),				\
> +		 __HYPERCALL_CLOBBER1);					\
>  	(type)__res;							\
>  })
>  
> @@ -168,10 +167,10 @@ extern struct { char _entry[32]; } hypercall_page[];
>  ({									\
>  	__HYPERCALL_DECLS;						\
>  	__HYPERCALL_2ARG(a1, a2);					\
> -	asm volatile (__HYPERCALL					\
> -		      : __HYPERCALL_2PARAM				\
> -		      : __HYPERCALL_ENTRY(name)				\
> -		      : __HYPERCALL_CLOBBER2);				\
> +	ASM_CALL(__HYPERCALL,						\
> +		 __HYPERCALL_2PARAM,					\
> +		 __HYPERCALL_ENTRY(name),				\
> +		 __HYPERCALL_CLOBBER2);					\
>  	(type)__res;							\
>  })
>  
> @@ -179,10 +178,10 @@ extern struct { char _entry[32]; } hypercall_page[];
>  ({									\
>  	__HYPERCALL_DECLS;						\
>  	__HYPERCALL_3ARG(a1, a2, a3);					\
> -	asm volatile (__HYPERCALL					\
> -		      : __HYPERCALL_3PARAM				\
> -		      : __HYPERCALL_ENTRY(name)				\
> -		      : __HYPERCALL_CLOBBER3);				\
> +	ASM_CALL(__HYPERCALL,						\
> +		 __HYPERCALL_3PARAM,					\
> +		 __HYPERCALL_ENTRY(name),				\
> +		 __HYPERCALL_CLOBBER3);					\
>  	(type)__res;							\
>  })
>  
> @@ -190,10 +189,10 @@ extern struct { char _entry[32]; } hypercall_page[];
>  ({									\
>  	__HYPERCALL_DECLS;						\
>  	__HYPERCALL_4ARG(a1, a2, a3, a4);				\
> -	asm volatile (__HYPERCALL					\
> -		      : __HYPERCALL_4PARAM				\
> -		      : __HYPERCALL_ENTRY(name)				\
> -		      : __HYPERCALL_CLOBBER4);				\
> +	ASM_CALL(__HYPERCALL,						\
> +		 __HYPERCALL_4PARAM,					\
> +		 __HYPERCALL_ENTRY(name),				\
> +		 __HYPERCALL_CLOBBER4);					\
>  	(type)__res;							\
>  })
>  
> @@ -201,10 +200,10 @@ extern struct { char _entry[32]; } hypercall_page[];
>  ({									\
>  	__HYPERCALL_DECLS;						\
>  	__HYPERCALL_5ARG(a1, a2, a3, a4, a5);				\
> -	asm volatile (__HYPERCALL					\
> -		      : __HYPERCALL_5PARAM				\
> -		      : __HYPERCALL_ENTRY(name)				\
> -		      : __HYPERCALL_CLOBBER5);				\
> +	ASM_CALL(__HYPERCALL,						\
> +		 __HYPERCALL_5PARAM,					\
> +		 __HYPERCALL_ENTRY(name),				\
> +		 __HYPERCALL_CLOBBER5);					\
>  	(type)__res;							\
>  })
>  
> @@ -218,10 +217,10 @@ privcmd_call(unsigned call,
>  	__HYPERCALL_5ARG(a1, a2, a3, a4, a5);
>  
>  	stac();
> -	asm volatile("call *%[call]"
> -		     : __HYPERCALL_5PARAM
> -		     : [call] "a" (&hypercall_page[call])
> -		     : __HYPERCALL_CLOBBER5);
> +	ASM_CALL("call *%[call]",
> +		 __HYPERCALL_5PARAM,
> +		[call] "a" (&hypercall_page[call]),
> +		__HYPERCALL_CLOBBER5);
>  	clac();
>  
>  	return (long)__res;
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index fb0055953fbc..4d0d326029a7 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5284,16 +5284,15 @@ static void fetch_possible_mmx_operand(struct x86_emulate_ctxt *ctxt,
>  
>  static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *))
>  {
> -	register void *__sp asm(_ASM_SP);
>  	ulong flags = (ctxt->eflags & EFLAGS_MASK) | X86_EFLAGS_IF;
>  
>  	if (!(ctxt->d & ByteOp))
>  		fop += __ffs(ctxt->dst.bytes) * FASTOP_SIZE;
>  
> -	asm("push %[flags]; popf; call *%[fastop]; pushf; pop %[flags]\n"
> -	    : "+a"(ctxt->dst.val), "+d"(ctxt->src.val), [flags]"+D"(flags),
> -	      [fastop]"+S"(fop), "+r"(__sp)
> -	    : "c"(ctxt->src2.val));
> +	ASM_CALL("push %[flags]; popf; call *%[fastop]; pushf; pop %[flags]\n",
> +		 ARGS("+a"(ctxt->dst.val), "+d"(ctxt->src.val),
> +		      [flags]"+D"(flags), [fastop]"+S"(fop)),
> +		 ARGS("c"(ctxt->src2.val)));
>  
>  	ctxt->eflags = (ctxt->eflags & ~EFLAGS_MASK) | (flags & EFLAGS_MASK);
>  	if (!fop) /* exception is returned in fop variable */
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ffd469ecad57..d42806ad3c9c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8671,7 +8671,6 @@ static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
>  static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
>  {
>  	u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
> -	register void *__sp asm(_ASM_SP);
>  
>  	if ((exit_intr_info & (INTR_INFO_VALID_MASK | INTR_INFO_INTR_TYPE_MASK))
>  			== (INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR)) {
> @@ -8686,7 +8685,7 @@ static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
>  		vector =  exit_intr_info & INTR_INFO_VECTOR_MASK;
>  		desc = (gate_desc *)vmx->host_idt_base + vector;
>  		entry = gate_offset(*desc);
> -		asm volatile(
> +		ASM_CALL(
>  #ifdef CONFIG_X86_64
>  			"mov %%" _ASM_SP ", %[sp]\n\t"
>  			"and $0xfffffffffffffff0, %%" _ASM_SP "\n\t"
> @@ -8695,16 +8694,14 @@ static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
>  #endif
>  			"pushf\n\t"
>  			__ASM_SIZE(push) " $%c[cs]\n\t"
> -			"call *%[entry]\n\t"
> -			:
> +			"call *%[entry]\n\t",
>  #ifdef CONFIG_X86_64
> -			[sp]"=&r"(tmp),
> +			[sp]"=&r"(tmp)
>  #endif
> -			"+r"(__sp)
> -			:
> -			[entry]"r"(entry),
> -			[ss]"i"(__KERNEL_DS),
> -			[cs]"i"(__KERNEL_CS)
> +			,
> +			ARGS([entry]"r"(entry),
> +			     [ss]"i"(__KERNEL_DS),
> +			     [cs]"i"(__KERNEL_CS))
>  			);
>  	}
>  }
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 2a1fa10c6a98..ca642ac3a390 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -802,7 +802,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
>  	if (is_vmalloc_addr((void *)address) &&
>  	    (((unsigned long)tsk->stack - 1 - address < PAGE_SIZE) ||
>  	     address - ((unsigned long)tsk->stack + THREAD_SIZE) < PAGE_SIZE)) {
> -		register void *__sp asm("rsp");
>  		unsigned long stack = this_cpu_read(orig_ist.ist[DOUBLEFAULT_STACK]) - sizeof(void *);
>  		/*
>  		 * We're likely to be running with very little stack space
> @@ -814,13 +813,12 @@ no_context(struct pt_regs *regs, unsigned long error_code,
>  		 * and then double-fault, though, because we're likely to
>  		 * break the console driver and lose most of the stack dump.
>  		 */
> -		asm volatile ("movq %[stack], %%rsp\n\t"
> -			      "call handle_stack_overflow\n\t"
> -			      "1: jmp 1b"
> -			      : "+r" (__sp)
> -			      : "D" ("kernel stack overflow (page fault)"),
> -				"S" (regs), "d" (address),
> -				[stack] "rm" (stack));
> +		ASM_CALL("movq %[stack], %%rsp\n\t"
> +			 "call handle_stack_overflow\n\t"
> +			 "1: jmp 1b",
> +			 ,
> +			 ARGS("D" ("kernel stack overflow (page fault)"),
> +			      "S" (regs), "d" (address), [stack] "rm" (stack)));
>  		unreachable();
>  	}
>  #endif
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 3f8c88e29a46..3a57f32a0bfd 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -589,4 +589,18 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>  	(_________p1); \
>  })
>  
> +#define ARGS(args...) args
> +
> +#ifdef CONFIG_FRAME_POINTER
> +
> +#define ASM_CALL(str, outputs, inputs, clobbers...) \
> +	asm volatile(str : outputs : inputs : "sp", ## clobbers)
> +
> +#else
> +
> +#define ASM_CALL(str, outputs, inputs, clobbers...) \
> +	asm volatile(str : outputs : inputs : clobbers);
> +
> +#endif
> +
>  #endif /* __LINUX_COMPILER_H */
> diff --git a/tools/objtool/Documentation/stack-validation.txt b/tools/objtool/Documentation/stack-validation.txt
> index 6a1af43862df..766a00ebf80c 100644
> --- a/tools/objtool/Documentation/stack-validation.txt
> +++ b/tools/objtool/Documentation/stack-validation.txt
> @@ -193,13 +193,8 @@ they mean, and suggestions for how to fix them.
>  
>     If it's a GCC-compiled .c file, the error may be because the function
>     uses an inline asm() statement which has a "call" instruction.  An
> -   asm() statement with a call instruction must declare the use of the
> -   stack pointer in its output operand.  For example, on x86_64:
> -
> -     register void *__sp asm("rsp");
> -     asm volatile("call func" : "+r" (__sp));
> -
> -   Otherwise the stack frame may not get created before the call.
> +   asm() statement with a call instruction must use the ASM_CALL macro,
> +   which forces the frame pointer to be saved before the call.
>  
>  
>  2. file.o: warning: objtool: .text+0x53: unreachable instruction
> @@ -221,7 +216,7 @@ they mean, and suggestions for how to fix them.
>     change ENDPROC to END.
>  
>  
> -4. file.o: warning: objtool: func(): can't find starting instruction
> +3. file.o: warning: objtool: func(): can't find starting instruction
>     or
>     file.o: warning: objtool: func()+0x11dd: can't decode instruction
>  
> @@ -230,7 +225,7 @@ they mean, and suggestions for how to fix them.
>     section like .data or .rodata.
>  
>  
> -5. file.o: warning: objtool: func()+0x6: unsupported instruction in callable function
> +4. file.o: warning: objtool: func()+0x6: unsupported instruction in callable function
>  
>     This is a kernel entry/exit instruction like sysenter or iret.  Such
>     instructions aren't allowed in a callable function, and are most
> @@ -239,7 +234,7 @@ they mean, and suggestions for how to fix them.
>     annotated with the unwind hint macros in asm/unwind_hints.h.
>  
>  
> -6. file.o: warning: objtool: func()+0x26: sibling call from callable instruction with modified stack frame
> +5. file.o: warning: objtool: func()+0x26: sibling call from callable instruction with modified stack frame
>  
>     This is a dynamic jump or a jump to an undefined symbol.  Objtool
>     assumed it's a sibling call and detected that the frame pointer
> @@ -253,7 +248,7 @@ they mean, and suggestions for how to fix them.
>     the unwind hint macros in asm/unwind_hints.h.
>  
>  
> -7. file: warning: objtool: func()+0x5c: stack state mismatch
> +6. file: warning: objtool: func()+0x5c: stack state mismatch
>  
>     The instruction's frame pointer state is inconsistent, depending on
>     which execution path was taken to reach the instruction.
> @@ -270,7 +265,7 @@ they mean, and suggestions for how to fix them.
>     asm/unwind_hints.h.
>  
>  
> -8. file.o: warning: objtool: funcA() falls through to next function funcB()
> +7. file.o: warning: objtool: funcA() falls through to next function funcB()
>  
>     This means that funcA() doesn't end with a return instruction or an
>     unconditional jump, and that objtool has determined that the function

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web