Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932121AbbGQRcX (ORCPT ); Fri, 17 Jul 2015 13:32:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55549 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752869AbbGQRcW (ORCPT ); Fri, 17 Jul 2015 13:32:22 -0400 Date: Fri, 17 Jul 2015 12:32:20 -0500 From: Josh Poimboeuf To: Borislav Petkov Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Michal Marek , Peter Zijlstra , Andy Lutomirski , Linus Torvalds , Andi Kleen , Pedro Alves , x86@kernel.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 04/21] x86/hweight: Add stack frame dependency for __arch_hweight*() Message-ID: <20150717173220.GA12761@treble.redhat.com> References: <0d8517b7ab757e00a13b3abe2b677d9eb23362be.1437150175.git.jpoimboe@redhat.com> <20150717171726.GA21568@nazgul.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150717171726.GA21568@nazgul.tnic> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2877 Lines: 71 On Fri, Jul 17, 2015 at 07:17:26PM +0200, Borislav Petkov wrote: > On Fri, Jul 17, 2015 at 11:47:20AM -0500, Josh Poimboeuf wrote: > > If __arch_hweight32() or __arch_hweight64() is inlined at the beginning > > of a function, gcc can insert the call instruction before setting up a > > stack frame, which breaks frame pointer convention if > > CONFIG_FRAME_POINTER is enabled and can result in a bad stack trace. > > > > Force a stack frame to be created if CONFIG_FRAME_POINTER is enabled by > > listing the stack pointer as an output operand for the inline asm > > statement. > > > > Signed-off-by: Josh Poimboeuf > > --- > > arch/x86/include/asm/arch_hweight.h | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arch_hweight.h > > index 9686c3d..e438a0d 100644 > > --- a/arch/x86/include/asm/arch_hweight.h > > +++ b/arch/x86/include/asm/arch_hweight.h > > @@ -23,10 +23,11 @@ > > */ > > static inline unsigned int __arch_hweight32(unsigned int w) > > { > > + register void *__sp asm("esp"); > > unsigned int res = 0; > > > > asm (ALTERNATIVE("call __sw_hweight32", POPCNT32, X86_FEATURE_POPCNT) > > - : "="REG_OUT (res) > > + : "="REG_OUT (res), "+r" (__sp) > > : REG_IN (w)); > > > > return res; > > @@ -44,6 +45,7 @@ static inline unsigned int __arch_hweight8(unsigned int w) > > > > static inline unsigned long __arch_hweight64(__u64 w) > > { > > + register void __maybe_unused *__sp asm("rsp"); > > unsigned long res = 0; > > > > #ifdef CONFIG_X86_32 > > @@ -51,7 +53,7 @@ static inline unsigned long __arch_hweight64(__u64 w) > > __arch_hweight32((u32)(w >> 32)); > > #else > > asm (ALTERNATIVE("call __sw_hweight64", POPCNT64, X86_FEATURE_POPCNT) > > - : "="REG_OUT (res) > > + : "="REG_OUT (res), "+r" (__sp) > > : REG_IN (w)); > > #endif /* CONFIG_X86_32 */ > > Eeew, useless code so that some compile-time validation is done. Let's > not add this clutter please. > > In this particular case, the majority of CPUs out there will get POPCNT > patched in and that CALL is gone. And for the remaining cases where we > do end up using the __sw_* variants, I'd prefer to rather not do the > validation instead of polluting the code with that fake rsp dependency. Well, but this isn't some whitelist code to make stackvalidate happy. It's actually a real runtime frame pointer bug, and the rsp dependency is real. If it does the call without first creating the stack frame then it breaks frame pointer based stack traces. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/