PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task

PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task

[Oleg Nesterov]

I didn't try to verify __switch_to()->__math_state_restore() is really
wrong, this is more the question than the patch. But at least the code
looks wrong, it calls __math_state_restore() which uses curent before
current_task was updated.

Uncompiled/untested.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -377,9 +377,6 @@ __switch_to(struct task_struct *prev_p, 
 	 */
 	arch_end_context_switch(next_p);
 
-	if (preload_fpu)
-		__math_state_restore();
-
 	/*
 	 * Restore %gs if needed (which is common)
 	 */
@@ -388,6 +385,9 @@ __switch_to(struct task_struct *prev_p, 
 
 	percpu_write(current_task, next_p);
 
+	if (preload_fpu)
+		__math_state_restore();
+
 	return prev_p;
 }
 

Re: PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task

[Suresh Siddha]

On Thu, 2010-02-04 at 08:51 -0800, Oleg Nesterov wrote:
> I didn't try to verify __switch_to()->__math_state_restore() is really
> wrong, this is more the question than the patch. But at least the code
> looks wrong, it calls __math_state_restore() which uses curent before
> current_task was updated.
> 
> Uncompiled/untested.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -377,9 +377,6 @@ __switch_to(struct task_struct *prev_p, 
>  	 */
>  	arch_end_context_switch(next_p);
>  
> -	if (preload_fpu)
> -		__math_state_restore();
> -
>  	/*
>  	 * Restore %gs if needed (which is common)
>  	 */
> @@ -388,6 +385,9 @@ __switch_to(struct task_struct *prev_p, 
>  
>  	percpu_write(current_task, next_p);
>  
> +	if (preload_fpu)
> +		__math_state_restore();
> +
>  	return prev_p;
>  }

Oleg, __math_state_restore() uses current_thread_info() which at that
point already has the right esp and as such uses the correct thread
struct etc.

After saying that, in the past I have also ran into this question and
got satisfied by looking deeper. Best is to make the 32bit and 64bit
code similar as much as possible and as such your patch is acceptable.

Can you please re-post with a proper changelog (and ofcourse testing
etc)? You can add my Ack to that.

thanks,
suresh

Re: PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task

[Oleg Nesterov]

On 02/04, Suresh Siddha wrote:
>
> On Thu, 2010-02-04 at 08:51 -0800, Oleg Nesterov wrote:
> >
> > --- a/arch/x86/kernel/process_32.c
> > +++ b/arch/x86/kernel/process_32.c
> > @@ -377,9 +377,6 @@ __switch_to(struct task_struct *prev_p,
> >  	 */
> >  	arch_end_context_switch(next_p);
> >
> > -	if (preload_fpu)
> > -		__math_state_restore();
> > -
> >  	/*
> >  	 * Restore %gs if needed (which is common)
> >  	 */
> > @@ -388,6 +385,9 @@ __switch_to(struct task_struct *prev_p,
> >
> >  	percpu_write(current_task, next_p);
> >
> > +	if (preload_fpu)
> > +		__math_state_restore();
> > +
> >  	return prev_p;
> >  }
>
> Oleg, __math_state_restore() uses current_thread_info() which at that
> point already has the right esp and as such uses the correct thread
> struct etc.

Ah, indeed. I didn't notice that, unlike X86_64, X86_32 uses esp, not
kernel_stack to get thread_info, and __math_state_restore() pathes never
use get_current().

> After saying that, in the past I have also ran into this question and
> got satisfied by looking deeper. Best is to make the 32bit and 64bit
> code similar as much as possible and as such your patch is acceptable.
>
> Can you please re-post with a proper changelog (and ofcourse testing
> etc)? You can add my Ack to that.

OK, will do once I have a 32bit machine to test.

Thanks a lot for your explanation!


Could you please explain me another issue? I know nothing about fpu
handling, I am reading this code trying to understand the unrelated
bug with the old kernel.

In short: why restore_i387_xstate() does init_fpu() when !used_math(),
can't (or shouldn't) it merely do set_used_math() ?

	restore_i387_xstate:
		
		if (!used_math()) {
			err = init_fpu(tsk);
			if (err)
				return err;
		}

note that buf != NULL. This means that used_math() was true when
get_sigframe() was called, otherwise buf == sigcontext->fpstate
would be NULL, right?

So, the task must have the valid ->thread.xstate, and init_fpu()
just resets the contents of *thread.xstate. Why? We are going to
reload the FPU registers and set TS_USEDFPU. This means .xstate
must be never used until we save the FPU registers back, correct?

IOW, I'd appreciate very much if you can explain me why the patch
below is wrong. To clarify, even if the patch is correct I do not
mean it is really needed, I am just trying to understand what I
have missed here.

Thanks,

Oleg.

--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -215,11 +215,7 @@ int restore_i387_xstate(void __user *buf
 		if (!access_ok(VERIFY_READ, buf, sig_xstate_size))
 			return -EACCES;
 
-	if (!used_math()) {
-		err = init_fpu(tsk);
-		if (err)
-			return err;
-	}
+	set_stopped_child_used_math(tsk);
 
 	if (!(task_thread_info(current)->status & TS_USEDFPU)) {
 		clts();

Re: PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task

[Oleg Nesterov]

On 02/05, Oleg Nesterov wrote:
>
> In short: why restore_i387_xstate() does init_fpu() when !used_math(),
> can't (or shouldn't) it merely do set_used_math() ?
>
> 	restore_i387_xstate:
> 		
> 		if (!used_math()) {
> 			err = init_fpu(tsk);
> 			if (err)
> 				return err;
> 		}
>
> note that buf != NULL. This means that used_math() was true when
> get_sigframe() was called, otherwise buf == sigcontext->fpstate
> would be NULL, right?
>
> So, the task must have the valid ->thread.xstate,

This is probably correct. But a malicious user can set sc->fpsate
and fool the kernel, so we must assume buf != NULL implies the
valid ->xstate.

Oleg.

Re: PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task

[Oleg Nesterov]

On 02/06, Oleg Nesterov wrote:
>
> On 02/05, Oleg Nesterov wrote:
> >
> > In short: why restore_i387_xstate() does init_fpu() when !used_math(),
> > can't (or shouldn't) it merely do set_used_math() ?
> >
> > 	restore_i387_xstate:
> > 		
> > 		if (!used_math()) {
> > 			err = init_fpu(tsk);
> > 			if (err)
> > 				return err;
> > 		}
> >
> > note that buf != NULL. This means that used_math() was true when
> > get_sigframe() was called, otherwise buf == sigcontext->fpstate
> > would be NULL, right?
> >
> > So, the task must have the valid ->thread.xstate,
>
> This is probably correct. But a malicious user can set sc->fpsate
> and fool the kernel, so we must assume buf != NULL implies the
                             ^^^^
                             can't
> valid ->xstate.

Oleg.

Re: PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task

[Suresh Siddha]

On Fri, 2010-02-05 at 04:44 -0800, Oleg Nesterov wrote:
> Could you please explain me another issue? I know nothing about fpu
> handling, I am reading this code trying to understand the unrelated
> bug with the old kernel.
> 
> In short: why restore_i387_xstate() does init_fpu() when !used_math(),
> can't (or shouldn't) it merely do set_used_math() ?
> 
> 	restore_i387_xstate:
> 		
> 		if (!used_math()) {
> 			err = init_fpu(tsk);
> 			if (err)
> 				return err;
> 		}
> 
> note that buf != NULL. This means that used_math() was true when
> get_sigframe() was called, otherwise buf == sigcontext->fpstate
> would be NULL, right?

No. When the signal is delivered sigcontext->fpstate can be null and
when we return from the signal handler, user can update the
sigcontext->fpstate pointer and the user expects the kernel to update
the fpstate of the process accordingly.

> So, the task must have the valid ->thread.xstate, 

Need not be.

thanks,
suresh

Re: PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task

[Oleg Nesterov]

On 02/08, Suresh Siddha wrote:
>
> On Fri, 2010-02-05 at 04:44 -0800, Oleg Nesterov wrote:
> > Could you please explain me another issue? I know nothing about fpu
> > handling, I am reading this code trying to understand the unrelated
> > bug with the old kernel.
> >
> > In short: why restore_i387_xstate() does init_fpu() when !used_math(),
> > can't (or shouldn't) it merely do set_used_math() ?
> >
> > 	restore_i387_xstate:
> >
> > 		if (!used_math()) {
> > 			err = init_fpu(tsk);
> > 			if (err)
> > 				return err;
> > 		}
> >
> > note that buf != NULL. This means that used_math() was true when
> > get_sigframe() was called, otherwise buf == sigcontext->fpstate
> > would be NULL, right?
>
> No. When the signal is delivered sigcontext->fpstate can be null and
> when we return from the signal handler, user can update the
> sigcontext->fpstate pointer

Yes, I already realized this, see my next emails.

> and the user expects the kernel to update
> the fpstate of the process accordingly.

but I didn't know it is "officially" allowed to populate .xstate
this way.


Thanks Suresh for your explanations.

Oleg.