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

[Josh Poimboeuf]

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