Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753424AbdG2Ai5 (ORCPT ); Fri, 28 Jul 2017 20:38:57 -0400 Received: from mail-pg0-f46.google.com ([74.125.83.46]:36209 "EHLO mail-pg0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753272AbdG2Aiz (ORCPT ); Fri, 28 Jul 2017 20:38:55 -0400 Date: Fri, 28 Jul 2017 17:38:52 -0700 From: Matthias Kaehlcke To: Josh Poimboeuf Cc: Andrey Ryabinin , Chris J Arges , Borislav Petkov , Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , "x86@kernel.org" , LKML , Douglas Anderson , Michael Davidson , Greg Hackmann , Nick Desaulniers , Stephen Hines , Kees Cook , Arnd Bergmann , Bernhard =?utf-8?Q?Rosenkr=C3=A4nzer?= Subject: Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm" Message-ID: <20170729003852.GD84665@google.com> References: <75850bb7-a60e-057d-d88b-afa0c79e94a0@gmail.com> <20170713203416.isvijqbwbcgupgj7@treble> <20170713211245.GG95735@google.com> <20170713213406.gx4ixkx6kxa4ppps@treble> <20170713215704.GJ95735@google.com> <20170719174630.kz5g553evcrnirmr@treble> <20170720151813.5wnpsb5wy7bqrpec@treble> <20170720205652.patvjnfqymrv73ho@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20170720205652.patvjnfqymrv73ho@treble> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3169 Lines: 71 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.