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

[Borislav Petkov]

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)
--