linux-kernel - Re: PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current

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]

Message-ID: <20100205124407.GA24974@redhat.com>
Date:	Fri, 5 Feb 2010 13:44:07 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Suresh Siddha <suresh.b.siddha@...el.com>
Cc:	Arjan van de Ven <arjan@...ux.intel.com>,
	Jeremy Fitzhardinge <jeremy.fitzhardinge@...rix.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: PATCH? process_32.c:__switch_to() calls __math_state_restore()
	before updating current_task

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();

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