linux-kernel - Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 May 2016 19:20:45 +0200
From:	Borislav Petkov <bp@...e.de>
To:	Yu-cheng Yu <yu-cheng.yu@...el.com>
Cc:	linux-kernel@...r.kernel.org, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Andy Lutomirski <luto@...nel.org>,
	Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>,
	"Ravi V. Shankar" <ravi.v.shankar@...el.com>,
	Fenghua Yu <fenghua.yu@...el.com>
Subject: Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size

On Tue, May 10, 2016 at 04:29:53PM -0700, Yu-cheng Yu wrote:
> The XSAVE area of kernel can be in standard or compacted format;

"The kernel xstate area... "

and can we call it the xstate area as there are a bunch of XSAVE*
insns touching it. The file which deals with it is even called that:
arch/x86/kernel/fpu/xstate.c

> it is always in standard format for user mode. When XSAVES is
> enabled, the kernel uses the compacted format and it is necessary
> to use a separate fpu_user_xstate_size for signal/ptrace frames.
> 
> Based on an earlier patch from Fenghua Yu <fenghua.yu@...el.com>
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@...el.com>
> [yu-cheng.yu@...el.com: rebase to current, rename to fpu_user_xstate_size]
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@...el.com>
> Reviewed-by: Dave Hansen <dave.hansen@...el.com>

Maybe I wasn't as clear as I hoped to be. Let me be more specific:

So you either need to do:

---
From: Fenghua

...

Signed-off-by: Fenghua
Signed-off-by: You
...
---

or

---

Based on an earlier patch from Fenghua Yu <fenghua.yu@...el.com>.

Signed-off-by: You

---

with the second variant making you the author implicitly because you're
the sender.

Makes more sense this way?

> ---
>  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      | 27 ++++++++++----
>  arch/x86/kernel/fpu/xstate.c      | 76 ++++++++++++++++++++++++---------------
>  5 files changed, 73 insertions(+), 37 deletions(-)

...

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index b48ef35..dfac87d 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -44,6 +44,13 @@ static unsigned int xstate_sizes[XFEATURE_MAX]   = { [ 0 ... XFEATURE_MAX - 1] =
>  static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
>  
>  /*
> + * The XSAVE area of kernel can be in standard or compacted format;

"The kernel xstate area... "

> + * it is always in standard format for user mode. This is the user
> + * mode standard format size used for signal and ptrace frames.
> + */

But yes, that's very nice commenting.

> +unsigned int fpu_user_xstate_size;
> +
> +/*
>   * Clear all of the X86_FEATURE_* bits that are unavailable
>   * when the CPU has no XSAVE support.
>   */

...

> @@ -591,7 +598,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)

So next time you submit patches against -tip, please merge them with
tip/master and see if they still build, at least.

During last review I told you to use boot_cpu_has(X86_FEATURE_XSAVES)
because those cpu_has_XXX things are going away and are gone in tip
already.

Please be more careful when incorporating review comments - otherwise it
is waste of both yours and reviewer's time.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--