Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965527AbdGTPa1 (ORCPT ); Thu, 20 Jul 2017 11:30:27 -0400 Received: from mail-qk0-f195.google.com ([209.85.220.195]:35102 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965158AbdGTPaZ (ORCPT ); Thu, 20 Jul 2017 11:30:25 -0400 MIME-Version: 1.0 In-Reply-To: <20170720151813.5wnpsb5wy7bqrpec@treble> References: <20170712232213.GE95735@google.com> <20170713180001.mvwzdmudht56hdk5@treble> <20170713184748.GF95735@google.com> <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> From: Andrey Ryabinin Date: Thu, 20 Jul 2017 18:30:24 +0300 Message-ID: Subject: Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm" To: Josh Poimboeuf Cc: Matthias Kaehlcke , 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 , =?UTF-8?Q?Bernhard_Rosenkr=C3=A4nzer?= Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2630 Lines: 60 2017-07-20 18:18 GMT+03:00 Josh Poimboeuf : > On Thu, Jul 20, 2017 at 01:01:39PM +0300, Andrey Ryabinin wrote: >> 2017-07-19 20:46 GMT+03:00 Josh Poimboeuf : >> >> > >> > 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