From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751350AbcEJLEs (ORCPT ); Tue, 10 May 2016 07:04:48 -0400 Received: from mx2.suse.de ([195.135.220.15]:38040 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750954AbcEJLEr (ORCPT ); Tue, 10 May 2016 07:04:47 -0400 Date: Tue, 10 May 2016 13:04:41 +0200 From: Borislav Petkov To: Yu-cheng Yu Cc: linux-kernel@vger.kernel.org, x86@kernel.org, "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , Dave Hansen , Andy Lutomirski , Sai Praneeth Prakhya , "Ravi V. Shankar" , Fenghua Yu Subject: Re: [PATCH v5 01/13] x86/xsaves: Define and use user_xstate_size for xstate size in signal context Message-ID: <20160510110441.GA28520@pd.tnic> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 09, 2016 at 01:45:58PM -0700, Yu-cheng Yu wrote: > If "xsaves" is enabled, kernel always uses compacted format of xsave area. > But user space still uses standard format of xsave area. Thus, xstate size > in kernel's xsave area is smaller than xstate size in user's xsave area. > The xstate in signal frame should be in standard format for user's signal > handler to access. > > In no "xsaves" case, xsave area in both user space and kernel space are in > standard format. Therefore, user's and kernel's xstate sizes are equal. > > In "xsaves" case, xsave area in user space is in standard format while > xsave area in kernel space is in compacted format. Therefore, kernel's > xstate size is smaller than user's xstate size. So this repeats what the first paragraph said. > So here is the problem: currently kernel assumes its own xstate size is > signal frame's xstate size. This is not a problem in no "xsaves" case. ^ the This whole text is missing a bunch of "the"s... > It is an issue in "xsaves" case because kernel's xstate size is smaller ^ ^ the the and so on... > than user's xstate size. In fpu__alloc_mathframe(), a smaller fpstate > buffer is allocated for the standard format xstate in signal frame. > Then kernel saves only part of xstate registers into this smaller > user's fpstate buffer and user will see part of the xstate registers in > signal context. Similar issue happens after returning from signal handler: > kernel will only restore part of xstate registers from user's fpstate > buffer in signal frame. > > This patch defines and uses user_xstate_size for xstate size in signal > frame. It's read from returned value in ebx from CPUID leaf 0x0D subleaf > 0x0. This is maximum size required by enabled states in XCR0 and may be > different from ecx when states at the end of the xsave area are not > enabled. This value indicates the size required for XSAVE to save all > supported user states in legacy/standard format. > > Signed-off-by: Fenghua Yu > Signed-off-by: Yu-cheng Yu That SOB chain needs clarification: if Fenghua is the author, the patch should contain his From: at the top. If it is based on an earlier patch from him, commit message should say: Based on an earlier patch from Fenghua... without the SOB. > Reviewed-by: Dave Hansen > --- > arch/x86/include/asm/fpu/xstate.h | 1 - > arch/x86/include/asm/processor.h | 1 + > arch/x86/kernel/fpu/init.c | 5 ++- > arch/x86/kernel/fpu/signal.c | 26 ++++++++++---- > arch/x86/kernel/fpu/xstate.c | 71 ++++++++++++++++++++++++--------------- > 5 files changed, 67 insertions(+), 37 deletions(-) > > diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h > index 38951b0..16df2c4 100644 > --- a/arch/x86/include/asm/fpu/xstate.h > +++ b/arch/x86/include/asm/fpu/xstate.h > @@ -39,7 +39,6 @@ > #define REX_PREFIX > #endif > > -extern unsigned int xstate_size; > extern u64 xfeatures_mask; > extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS]; > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index 9264476..132b4ca 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -368,6 +368,7 @@ DECLARE_PER_CPU(struct irq_stack *, softirq_stack); > #endif /* X86_64 */ > > extern unsigned int xstate_size; > +extern unsigned int user_xstate_size; If this is going to be exported, let's prefix it pls: fpu_user_xstate_size or xstate_user_state_size or somesuch. And let's add a comment over its definition what exactly it represents. I.e., the aspect about the signal frame... ... > @@ -591,7 +593,15 @@ static bool is_supported_xstate_size(unsigned int test_xstate_size) > static int init_xstate_size(void) > { > /* Recompute the context size for enabled features: */ > - unsigned int possible_xstate_size = calculate_xstate_size(); > + unsigned int possible_xstate_size; > + unsigned int xsave_size; > + > + xsave_size = get_xsave_size(); > + > + if (cpu_has_xsaves) if (boot_cpu_has(X86_FEATURE_XSAVES)) > + possible_xstate_size = get_xsaves_size(); > + else > + possible_xstate_size = xsave_size; > > /* Ensure we have the space to store all enabled: */ > if (!is_supported_xstate_size(possible_xstate_size)) -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --