[PATCH] x86/fpu: Fix invalid FPU ptrace state after execve

[PATCH] x86/fpu: Fix invalid FPU ptrace state after execve

[Yu-cheng Yu]

Robert O'Callahan reported that after an execve PTRACE_GETREGSET
NT_X86_XSTATE continues to return the pre-exec register values
until the exec'ed task modifies FPU state.  The test code is at
https://bugzilla.redhat.com/attachment.cgi?id=1164286.

What is happening is when eagerfpu is enabled, fpu__clear() did
not properly clear fpstate.  Fix it by doing just that.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reported-by: Robert O'Callahan <robert@ocallahan.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: David Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>
---
 arch/x86/kernel/fpu/core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 4700401..4c203c4 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -521,14 +521,14 @@ void fpu__clear(struct fpu *fpu)
 {
 	WARN_ON_FPU(fpu != &current->thread.fpu); /* Almost certainly an anomaly */
 
-	if (!use_eager_fpu() || !static_cpu_has(X86_FEATURE_FPU)) {
-		/* FPU state will be reallocated lazily at the first use. */
-		fpu__drop(fpu);
-	} else {
-		if (!fpu->fpstate_active) {
-			fpu__activate_curr(fpu);
-			user_fpu_begin();
-		}
+	fpu__drop(fpu);
+
+	/*
+	 * When eagerfpu is used, make sure fpstate is cleared and initialized.
+	 */
+	if (use_eager_fpu()) {
+		fpu__activate_curr(fpu);
+		user_fpu_begin();
 		copy_init_fpstate_to_fpregs();
 	}
 }
-- 
1.9.1

Re: [PATCH] x86/fpu: Fix invalid FPU ptrace state after execve

[Borislav Petkov]

On Wed, Nov 16, 2016 at 08:56:36AM -0800, Yu-cheng Yu wrote:
> Robert O'Callahan reported that after an execve PTRACE_GETREGSET
> NT_X86_XSTATE continues to return the pre-exec register values
> until the exec'ed task modifies FPU state.  The test code is at
> https://bugzilla.redhat.com/attachment.cgi?id=1164286.
> 
> What is happening is when eagerfpu is enabled, fpu__clear() did
> not properly clear fpstate.  Fix it by doing just that.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Reported-by: Robert O'Callahan <robert@ocallahan.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: David Hansen <dave.hansen@linux.intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>
> ---
>  arch/x86/kernel/fpu/core.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 4700401..4c203c4 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -521,14 +521,14 @@ void fpu__clear(struct fpu *fpu)
>  {
>  	WARN_ON_FPU(fpu != &current->thread.fpu); /* Almost certainly an anomaly */
>  
> -	if (!use_eager_fpu() || !static_cpu_has(X86_FEATURE_FPU)) {
> -		/* FPU state will be reallocated lazily at the first use. */
> -		fpu__drop(fpu);
> -	} else {
> -		if (!fpu->fpstate_active) {
> -			fpu__activate_curr(fpu);
> -			user_fpu_begin();
> -		}
> +	fpu__drop(fpu);
> +
> +	/*
> +	 * When eagerfpu is used, make sure fpstate is cleared and initialized.
> +	 */
> +	if (use_eager_fpu()) {

c592b5734706 ("x86/fpu: Remove use_eager_fpu()")

Please redo this patch against tip/master.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/fpu: Fix invalid FPU ptrace state after execve

[Dave Hansen]

On 11/16/2016 08:56 AM, Yu-cheng Yu wrote:
> Robert O'Callahan reported that after an execve PTRACE_GETREGSET
> NT_X86_XSTATE continues to return the pre-exec register values
> until the exec'ed task modifies FPU state.  The test code is at
> https://bugzilla.redhat.com/attachment.cgi?id=1164286.
> 
> What is happening is when eagerfpu is enabled, fpu__clear() did
> not properly clear fpstate.  Fix it by doing just that.

Functionally, I think the patch is fine.  just a few
comment/documentation nits.

I think fpu__clear()'s comments are a bit out of date.  Could we make it
clear that it is invalidating both fpregs *and* fpstate?

I also think the

	/* FPU state will be reallocated lazily at the first use. */"

comment was fairly valuable.  Could we find some way to keep it?

The new comment:

> +	/*
> +	 * When eagerfpu is used, make sure fpstate is cleared and initialized.
> +	 */

also kinda implies that the if() block is only messing with fpstate.
Could we make that more clear?  Maybe by commenting the individual lines
inside the if():

> +	if (use_eager_fpu()) {
> +		fpu__activate_curr(fpu);
> +		user_fpu_begin();

instead of having it above?  Maybe something like:

	if (use_eager_fpu()) {
		/* activate and load init fpstate into 'fpu' */
		fpu__activate_curr(fpu);
		/* re-activate fpregs: */
		user_fpu_begin();
		/* take new init fpstate and place in fpregs: */
 		copy_init_fpstate_to_fpregs();
 	}

Re: [PATCH] x86/fpu: Fix invalid FPU ptrace state after execve

[Yu-cheng Yu]

On Thu, Nov 17, 2016 at 01:31:57PM -0800, Dave Hansen wrote:
> Functionally, I think the patch is fine.  just a few
> comment/documentation nits.

Thanks!  I will wait for a few days for any comments that might come up
and fix all together.

Yu-cheng

Re: [PATCH] x86/fpu: Fix invalid FPU ptrace state after execve

[Ingo Molnar]

* Dave Hansen <dave.hansen@linux.intel.com> wrote:

> On 11/16/2016 08:56 AM, Yu-cheng Yu wrote:
> > Robert O'Callahan reported that after an execve PTRACE_GETREGSET
> > NT_X86_XSTATE continues to return the pre-exec register values
> > until the exec'ed task modifies FPU state.  The test code is at
> > https://bugzilla.redhat.com/attachment.cgi?id=1164286.
> > 
> > What is happening is when eagerfpu is enabled, fpu__clear() did
> > not properly clear fpstate.  Fix it by doing just that.
> 
> Functionally, I think the patch is fine.  just a few
> comment/documentation nits.
> 
> I think fpu__clear()'s comments are a bit out of date.  Could we make it
> clear that it is invalidating both fpregs *and* fpstate?
> 
> I also think the
> 
> 	/* FPU state will be reallocated lazily at the first use. */"
> 
> comment was fairly valuable.  Could we find some way to keep it?
> 
> The new comment:
> 
> > +	/*
> > +	 * When eagerfpu is used, make sure fpstate is cleared and initialized.
> > +	 */
> 
> also kinda implies that the if() block is only messing with fpstate.
> Could we make that more clear?  Maybe by commenting the individual lines
> inside the if():
> 
> > +	if (use_eager_fpu()) {
> > +		fpu__activate_curr(fpu);
> > +		user_fpu_begin();
> 
> instead of having it above?  Maybe something like:
> 
> 	if (use_eager_fpu()) {
> 		/* activate and load init fpstate into 'fpu' */
> 		fpu__activate_curr(fpu);
> 		/* re-activate fpregs: */
> 		user_fpu_begin();
> 		/* take new init fpstate and place in fpregs: */
>  		copy_init_fpstate_to_fpregs();
>  	}

I agree with these suggestions - but I'll apply the simple patch to x86/urgent - 
which can then be backported as far as necessary, and then resolve the conflict 
with the v4.10 tip:x86/fpu branch, and on top of that we can fix these details, 
ok?

In particular I don't like it how non-obvious the semantics are from the function 
names. I think we should try to improve the nomenclature instead of adding 
comments to every line.

Thanks,

	Ingo