Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1391374 > unrolled thread

Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Dave Hansen <dave.hansen@linux.intel.com> - 2016-04-29 22:10 +0200
    Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-04-30 00:50 +0200
      Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Dave Hansen <dave.hansen@linux.intel.com> - 2016-04-30 02:40 +0200
        Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-02 18:10 +0200
          Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-02 18:40 +0200
            Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-02 18:50 +0200
              Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-02 19:30 +0200
                Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-02 19:40 +0200
                  Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-02 23:30 +0200
                    Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-02 23:30 +0200
                      Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-02 23:40 +0200
                        Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-03 00:30 +0200
                          Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-03 00:40 +0200
        Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows  correct xstates copy from kernel to user directly Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-02 18:10 +0200

#1391374 — Re: [PATCH v4 04/10] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

On 03/04/2016 10:12 AM, Yu-cheng Yu wrote:
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 0fbf60c..09945f1 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -130,6 +130,45 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
>  	return err;
>  }
>  
> +static int may_copy_fpregs_to_sigframe(void)
> +{
> +	/*
> +	 * In signal handling path, the kernel already checks if
> +	 * FPU instructions have been used before it calls
> +	 * copy_fpstate_to_sigframe(). We check this here again
> +	 * to detect any potential mis-use and saving invalid
> +	 * register values directly to a signal frame.
> +	 */
> +	WARN_ONCE(!current->thread.fpu.fpstate_active,
> +		  "direct FPU save with no math use\n");

This is probably an OK check for this _particular_ context (since this
context is all ready to copy_to_user() the fpu state).  But is it good
generally?  Why couldn't you have a !fpstate_active thread that _was_
fpregs_active?

Such a thread _could_ do a direct XSAVE with no issues.

[toc] | [next] | [standalone]

#1391510

On Fri, Apr 29, 2016 at 01:09:07PM -0700, Dave Hansen wrote:
> On 03/04/2016 10:12 AM, Yu-cheng Yu wrote:
> > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> > index 0fbf60c..09945f1 100644
> > --- a/arch/x86/kernel/fpu/signal.c
> > +++ b/arch/x86/kernel/fpu/signal.c
> > @@ -130,6 +130,45 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
> >  	return err;
> >  }
> >  
> > +static int may_copy_fpregs_to_sigframe(void)
> > +{
> > +	/*
> > +	 * In signal handling path, the kernel already checks if
> > +	 * FPU instructions have been used before it calls
> > +	 * copy_fpstate_to_sigframe(). We check this here again
> > +	 * to detect any potential mis-use and saving invalid
> > +	 * register values directly to a signal frame.
> > +	 */
> > +	WARN_ONCE(!current->thread.fpu.fpstate_active,
> > +		  "direct FPU save with no math use\n");
> 
> This is probably an OK check for this _particular_ context (since this
> context is all ready to copy_to_user() the fpu state).  But is it good
> generally?  Why couldn't you have a !fpstate_active thread that _was_
> fpregs_active?
> 
> Such a thread _could_ do a direct XSAVE with no issues.

But it won't come to this function unless fpstate_active is ture?

[toc] | [prev] | [next] | [standalone]

#1391542

On 04/29/2016 03:43 PM, Yu-cheng Yu wrote:
> On Fri, Apr 29, 2016 at 01:09:07PM -0700, Dave Hansen wrote:
>> On 03/04/2016 10:12 AM, Yu-cheng Yu wrote:
>>> +static int may_copy_fpregs_to_sigframe(void)
>>> +{
>>> +	/*
>>> +	 * In signal handling path, the kernel already checks if
>>> +	 * FPU instructions have been used before it calls
>>> +	 * copy_fpstate_to_sigframe(). We check this here again
>>> +	 * to detect any potential mis-use and saving invalid
>>> +	 * register values directly to a signal frame.
>>> +	 */
>>> +	WARN_ONCE(!current->thread.fpu.fpstate_active,
>>> +		  "direct FPU save with no math use\n");
>>
>> This is probably an OK check for this _particular_ context (since this
>> context is all ready to copy_to_user() the fpu state).  But is it good
>> generally?  Why couldn't you have a !fpstate_active thread that _was_
>> fpregs_active?
>>
>> Such a thread _could_ do a direct XSAVE with no issues.
> 
> But it won't come to this function unless fpstate_active is ture?

If may_copy_fpregs_to_sigframe() were called from a slightly different
context, or if we change the call-site, what breaks?

In other words. if we can still "may_copy_fpregs_to_sigframe()" no
matter the state of fpu.fpstate_active, then I don't think we should be
checking it in may_copy_fpregs_to_sigframe().

[toc] | [prev] | [next] | [standalone]

#1392340

On 05/02/2016 08:57 AM, Yu-cheng Yu wrote:
> On Fri, Apr 29, 2016 at 05:36:48PM -0700, Dave Hansen wrote:
>> If may_copy_fpregs_to_sigframe() were called from a slightly different
>> context, or if we change the call-site, what breaks?
>>
>> In other words. if we can still "may_copy_fpregs_to_sigframe()" no
>> matter the state of fpu.fpstate_active, then I don't think we should be
>> checking it in may_copy_fpregs_to_sigframe().
> 
> Do you mean, don't check fpu.fpstate_active here?

Not really.  I'm asking *why* the check is there.

[toc] | [prev] | [next] | [standalone]

#1392368

On Mon, May 02, 2016 at 09:06:41AM -0700, Dave Hansen wrote:
> On 05/02/2016 08:57 AM, Yu-cheng Yu wrote:
> > On Fri, Apr 29, 2016 at 05:36:48PM -0700, Dave Hansen wrote:
> >> If may_copy_fpregs_to_sigframe() were called from a slightly different
> >> context, or if we change the call-site, what breaks?
> >>
> >> In other words. if we can still "may_copy_fpregs_to_sigframe()" no
> >> matter the state of fpu.fpstate_active, then I don't think we should be
> >> checking it in may_copy_fpregs_to_sigframe().
> > 
> > Do you mean, don't check fpu.fpstate_active here?
> 
> Not really.  I'm asking *why* the check is there.

If (fpu.fpstate_active == 0), then the task does not use FPU; we don't
want to save these registers, right?  

[toc] | [prev] | [next] | [standalone]

#1392375

On 05/02/2016 09:34 AM, Yu-cheng Yu wrote:
> On Mon, May 02, 2016 at 09:06:41AM -0700, Dave Hansen wrote:
>> On 05/02/2016 08:57 AM, Yu-cheng Yu wrote:
>>> On Fri, Apr 29, 2016 at 05:36:48PM -0700, Dave Hansen wrote:
>>>> If may_copy_fpregs_to_sigframe() were called from a slightly different
>>>> context, or if we change the call-site, what breaks?
>>>>
>>>> In other words. if we can still "may_copy_fpregs_to_sigframe()" no
>>>> matter the state of fpu.fpstate_active, then I don't think we should be
>>>> checking it in may_copy_fpregs_to_sigframe().
>>>
>>> Do you mean, don't check fpu.fpstate_active here?
>>
>> Not really.  I'm asking *why* the check is there.
> 
> If (fpu.fpstate_active == 0), then the task does not use FPU; we don't
> want to save these registers, right?  

No.  It's possible to have fpstate_active=0 while fpregs_active=1.  Such
a task uses the FPU, but just hasn't done an XSAVE* to save the register
content to the fpstate buffer.

Note, this is just theoretical, and does not happen in this particular
call path today.

[toc] | [prev] | [next] | [standalone]

#1392386

On Mon, May 02, 2016 at 09:43:47AM -0700, Dave Hansen wrote:
> > If (fpu.fpstate_active == 0), then the task does not use FPU; we don't
> > want to save these registers, right?  
> 
> No.  It's possible to have fpstate_active=0 while fpregs_active=1.  Such
> a task uses the FPU, but just hasn't done an XSAVE* to save the register
> content to the fpstate buffer.
> 
> Note, this is just theoretical, and does not happen in this particular
> call path today.

What about...

static int may_copy_fpregs_to_sigframe(void)
{
	if (fpregs_active())
		return 1;


	WARN_ONCE(!current->thread.fpu.fpstate_active,
		  "direct FPU save with no math use\n");

	if (boot_cpu_has(X86_FEATURE_XSAVES))
		return 1;

	return 0;
} 

[toc] | [prev] | [next] | [standalone]

#1392396

On 05/02/2016 10:19 AM, Yu-cheng Yu wrote:
> On Mon, May 02, 2016 at 09:43:47AM -0700, Dave Hansen wrote:
>>> If (fpu.fpstate_active == 0), then the task does not use FPU; we don't
>>> want to save these registers, right?  
>>
>> No.  It's possible to have fpstate_active=0 while fpregs_active=1.  Such
>> a task uses the FPU, but just hasn't done an XSAVE* to save the register
>> content to the fpstate buffer.
>>
>> Note, this is just theoretical, and does not happen in this particular
>> call path today.
> 
> What about...
> 
> static int may_copy_fpregs_to_sigframe(void)
> {
> 	if (fpregs_active())
> 		return 1;
> 
> 	WARN_ONCE(!current->thread.fpu.fpstate_active,
> 		  "direct FPU save with no math use\n");
> 
> 	if (boot_cpu_has(X86_FEATURE_XSAVES))
> 		return 1;
> 
> 	return 0;
> } 

I don't think that changes anything.  We still have a check in there
that has no purpose.  You've changed the ordering so that the specific
example that I pointed out no longer triggers it.  But, the underlying
issue remains.

[toc] | [prev] | [next] | [standalone]

#1392523

On Mon, May 02, 2016 at 10:33:10AM -0700, Dave Hansen wrote:
> On 05/02/2016 10:19 AM, Yu-cheng Yu wrote:
> > On Mon, May 02, 2016 at 09:43:47AM -0700, Dave Hansen wrote:
> >>> If (fpu.fpstate_active == 0), then the task does not use FPU; we don't
> >>> want to save these registers, right?  
> >>
> >> No.  It's possible to have fpstate_active=0 while fpregs_active=1.  Such
> >> a task uses the FPU, but just hasn't done an XSAVE* to save the register
> >> content to the fpstate buffer.
> >>
> >> Note, this is just theoretical, and does not happen in this particular
> >> call path today.
> > 
> > What about...
> > 
> > static int may_copy_fpregs_to_sigframe(void)
> > {
> > 	if (fpregs_active())
> > 		return 1;
> > 
> > 	WARN_ONCE(!current->thread.fpu.fpstate_active,
> > 		  "direct FPU save with no math use\n");
> > 
> > 	if (boot_cpu_has(X86_FEATURE_XSAVES))
> > 		return 1;
> > 
> > 	return 0;
> > } 
> 
> I don't think that changes anything.  We still have a check in there
> that has no purpose.  You've changed the ordering so that the specific
> example that I pointed out no longer triggers it.  But, the underlying
> issue remains.

Before Linux gets into copy_fpstate_to_sigframe(),
current->thread.fpu.fpstate_active must be true.
For eagerfpu, fpregs_active() must also be true.
For lazyfpu, once we try to do FSAVE/FXSAVE/XSAVE,
fpregs_active() will become true as well.

We should have not based on boot_cpu_has(X86_FEATURE_XSAVES)
at all. 

Why don't we make it simple and always copy_fpregs_to_signal_frame()?
Or, only for the lazy case, i.e. !fpregs_active(), we do __copy_to_user().

Anyway, I think we can just replace may_copy_fpregs_to_sigframe() with
!fpregs_active().

Comments? 

[toc] | [prev] | [next] | [standalone]

#1392525

On Mon, May 02, 2016 at 02:18:17PM -0700, Yu-cheng Yu wrote:
> Before Linux gets into copy_fpstate_to_sigframe(),
> current->thread.fpu.fpstate_active must be true.
> For eagerfpu, fpregs_active() must also be true.
> For lazyfpu, once we try to do FSAVE/FXSAVE/XSAVE,
> fpregs_active() will become true as well.
> 
> We should have not based on boot_cpu_has(X86_FEATURE_XSAVES)
> at all. 
> 
> Why don't we make it simple and always copy_fpregs_to_signal_frame()?
> Or, only for the lazy case, i.e. !fpregs_active(), we do __copy_to_user().

For (lazy && not XSAVES) actually!

[toc] | [prev] | [next] | [standalone]

#1392526

On 05/02/2016 02:24 PM, Yu-cheng Yu wrote:
> On Mon, May 02, 2016 at 02:18:17PM -0700, Yu-cheng Yu wrote:
>> > Before Linux gets into copy_fpstate_to_sigframe(),
>> > current->thread.fpu.fpstate_active must be true.
>> > For eagerfpu, fpregs_active() must also be true.
>> > For lazyfpu, once we try to do FSAVE/FXSAVE/XSAVE,
>> > fpregs_active() will become true as well.
>> > 
>> > We should have not based on boot_cpu_has(X86_FEATURE_XSAVES)
>> > at all. 
>> > 
>> > Why don't we make it simple and always copy_fpregs_to_signal_frame()?
>> > Or, only for the lazy case, i.e. !fpregs_active(), we do __copy_to_user().
> For (lazy && not XSAVES) actually!

I think we're off in the weeds here.

Please just send an updated patch for what you want to do here.

[toc] | [prev] | [next] | [standalone]

#1392549

On Mon, May 02, 2016 at 02:32:14PM -0700, Dave Hansen wrote:
> 
> I think we're off in the weeds here.
> 
> Please just send an updated patch for what you want to do here.

From 43134a773d23ae8bab9f158d143c5cfb76bc0e9c Mon Sep 17 00:00:00 2001
From: Yu-cheng Yu <yu-cheng.yu@intel.com>
Date: Sat, 14 Nov 2015 16:59:45 -0800
Subject: [PATCH] x86/xsaves: Introduce a new check that allows correct xstates
 copy from kernel to user directly

XSAVES is a kernel instruction and uses a compacted format. When working with user space, the kernel should provide
standard-format, non-supervisor state data. We cannot do __copy_to_user() from a compacted- format kernel xstate area to a
signal frame.

Dave Hansen proposes this method to simplify copy xstate directly to user.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/include/asm/fpu/xstate.h | 1 +
 arch/x86/kernel/fpu/signal.c      | 3 ++-
 arch/x86/kernel/fpu/xstate.c      | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 16df2c4..d812cf3 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -47,5 +47,6 @@ extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
+int using_compacted_format(void);
 
 #endif
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 0fbf60c..d7fdd8c 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -8,6 +8,7 @@
 #include <asm/fpu/internal.h>
 #include <asm/fpu/signal.h>
 #include <asm/fpu/regset.h>
+#include <asm/fpu/xstate.h>
 
 #include <asm/sigframe.h>
 
@@ -167,7 +168,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 			sizeof(struct user_i387_ia32_struct), NULL,
 			(struct _fpstate_32 __user *) buf) ? -1 : 1;
 
-	if (fpregs_active()) {
+	if (fpregs_active() || using_compacted_format()) {
 		/* Save the live register state to the user directly. */
 		if (copy_fpregs_to_sigframe(buf_fx))
 			return -1;
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 170c164..2b59bd7 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -415,7 +415,7 @@ static int xfeature_size(int xfeature_nr)
  * that it is obvious which aspect of 'XSAVES' is being handled
  * by the calling code.
  */
-static int using_compacted_format(void)
+int using_compacted_format(void)
 {
 	return cpu_has_xsaves;
 }
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1392551

On 05/02/2016 03:17 PM, Yu-cheng Yu wrote:
> @@ -167,7 +168,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
>  			sizeof(struct user_i387_ia32_struct), NULL,
>  			(struct _fpstate_32 __user *) buf) ? -1 : 1;
>  
> -	if (fpregs_active()) {
> +	if (fpregs_active() || using_compacted_format()) {
>  		/* Save the live register state to the user directly. */
>  		if (copy_fpregs_to_sigframe(buf_fx))
>  			return -1;

So, compared to the first patch, you move the fpregs_active() check out
to the caller of may_copy_fpregs_to_sigframe() (good) and removed a
bunch of comments explaining what was going on (bad).

Do we really want all those comments to die?

[toc] | [prev] | [next] | [standalone]

#1392343

On Fri, Apr 29, 2016 at 05:36:48PM -0700, Dave Hansen wrote:
> If may_copy_fpregs_to_sigframe() were called from a slightly different
> context, or if we change the call-site, what breaks?
> 
> In other words. if we can still "may_copy_fpregs_to_sigframe()" no
> matter the state of fpu.fpstate_active, then I don't think we should be
> checking it in may_copy_fpregs_to_sigframe().

Do you mean, don't check fpu.fpstate_active here?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web