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@xxxxxxxxxx>
> 
> --- 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/