Re: [PATCH v5 01/13] x86/xsaves: Define and use user_xstate_size for xstate size in signal context

[Yu-cheng Yu]

On Tue, May 10, 2016 at 01:04:41PM +0200, Borislav Petkov wrote:
> 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 <fenghua.yu@xxxxxxxxx>
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> 
> 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 <dave.hansen@xxxxxxxxx>
> > ---
> >  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)
> -- 

This is actually a patch of Fenghua's, but I re-based it.
I will fix it.

Yu-cheng