[PATCH 2.5] clear USEDFPU in copy_thread

[PATCH 2.5] clear USEDFPU in copy_thread

[Vivien Chappelier]

Hi,

	When copying a thread, access to the FPU is disabled by clearing
the ST0_CU1 bit in the new thread saved CP0_STATUS register. However, the
corresponding TIF_USEDFPU flag is not cleared at it should to indicate the
FPU has not yet been used by the new process.
	This patch also clears user_tid in mips64 code, and moves it away
from the FPU comment in the mips code.

Vivien.

Index: arch/mips64/kernel/process.c
===================================================================
RCS file: /home/cvs/linux/arch/mips64/kernel/process.c,v
retrieving revision 1.36
diff -u -r1.36 process.c
--- arch/mips64/kernel/process.c	9 Jan 2003 19:16:50 -0000	1.36
+++ arch/mips64/kernel/process.c	4 Feb 2003 22:47:00 -0000
@@ -114,6 +114,8 @@
 	p->thread.reg29 = (unsigned long) childregs;
 	p->thread.reg31 = (unsigned long) ret_from_fork;
 
+	p->user_tid = NULL;
+
 	/*
 	 * New tasks loose permission to use the fpu. This accelerates context
 	 * switching for most programs since they don't use the fpu.
@@ -121,6 +123,7 @@
 	p->thread.cp0_status = read_c0_status() &
                             ~(ST0_CU3|ST0_CU2|ST0_CU1|ST0_KSU);
 	childregs->cp0_status &= ~(ST0_CU3|ST0_CU2|ST0_CU1);
+	clear_ti_thread_flag(ti, TIF_USEDFPU);
 
 	return 0;
 }
Index: arch/mips/kernel/process.c
===================================================================
RCS file: /home/cvs/linux/arch/mips/kernel/process.c,v
retrieving revision 1.50
diff -u -r1.50 process.c
--- arch/mips/kernel/process.c	9 Jan 2003 19:16:50 -0000	1.50
+++ arch/mips/kernel/process.c	4 Feb 2003 22:47:04 -0000
@@ -117,6 +117,8 @@
 	p->thread.reg29 = (unsigned long) childregs;
 	p->thread.reg31 = (unsigned long) ret_from_fork;
 
+	p->user_tid = NULL;
+
 	/*
 	 * New tasks loose permission to use the fpu. This accelerates context
 	 * switching for most programs since they don't use the fpu.
@@ -124,7 +126,7 @@
 	p->thread.cp0_status = read_c0_status() &
                             ~(ST0_CU2|ST0_CU1|KU_MASK);
 	childregs->cp0_status &= ~(ST0_CU2|ST0_CU1);
-	p->user_tid = NULL;
+	clear_ti_thread_flag(ti, TIF_USEDFPU);
 
 	return 0;
 }

Re: [PATCH 2.5] clear USEDFPU in copy_thread

[Jun Sun]

You should *not* clear USEDFPU in copy_thread().  Imagine
you assign a floating point variable f=0.3, then do fork()
and then in the child process, alas, f is undefined (zero
most likely).

If you really want to do it, it should be in start_thread().

Even if you don't have it cleared in start_thread(), things
should be generally OK.  You will have some dirty FPU content
instead of a all-zero one when you start a new program.  But then
since all sane program should assign register values before they
first time use them, so this bug should be well hidden.

I am still curious whether this is a bug in i386 as well or they do
clear the flag in some non-obvious way.  Note that the unlazy_fpu()
in copy_thread won't do it.  It only clears the current process's
USEDFPU flag, while the child process's flag is set way back in
copy_flags() calls inside do_fork().


Jun


On Tue, Feb 04, 2003 at 11:54:38PM +0100, Vivien Chappelier wrote:
> Hi,
> 
> 	When copying a thread, access to the FPU is disabled by clearing
> the ST0_CU1 bit in the new thread saved CP0_STATUS register. However, the
> corresponding TIF_USEDFPU flag is not cleared at it should to indicate the
> FPU has not yet been used by the new process.
> 	This patch also clears user_tid in mips64 code, and moves it away
> from the FPU comment in the mips code.
> 
> Vivien.
> 
> Index: arch/mips64/kernel/process.c
> ===================================================================
> RCS file: /home/cvs/linux/arch/mips64/kernel/process.c,v
> retrieving revision 1.36
> diff -u -r1.36 process.c
> --- arch/mips64/kernel/process.c	9 Jan 2003 19:16:50 -0000	1.36
> +++ arch/mips64/kernel/process.c	4 Feb 2003 22:47:00 -0000
> @@ -114,6 +114,8 @@
>  	p->thread.reg29 = (unsigned long) childregs;
>  	p->thread.reg31 = (unsigned long) ret_from_fork;
>  
> +	p->user_tid = NULL;
> +
>  	/*
>  	 * New tasks loose permission to use the fpu. This accelerates context
>  	 * switching for most programs since they don't use the fpu.
> @@ -121,6 +123,7 @@
>  	p->thread.cp0_status = read_c0_status() &
>                              ~(ST0_CU3|ST0_CU2|ST0_CU1|ST0_KSU);
>  	childregs->cp0_status &= ~(ST0_CU3|ST0_CU2|ST0_CU1);
> +	clear_ti_thread_flag(ti, TIF_USEDFPU);
>  
>  	return 0;
>  }
> Index: arch/mips/kernel/process.c
> ===================================================================
> RCS file: /home/cvs/linux/arch/mips/kernel/process.c,v
> retrieving revision 1.50
> diff -u -r1.50 process.c
> --- arch/mips/kernel/process.c	9 Jan 2003 19:16:50 -0000	1.50
> +++ arch/mips/kernel/process.c	4 Feb 2003 22:47:04 -0000
> @@ -117,6 +117,8 @@
>  	p->thread.reg29 = (unsigned long) childregs;
>  	p->thread.reg31 = (unsigned long) ret_from_fork;
>  
> +	p->user_tid = NULL;
> +
>  	/*
>  	 * New tasks loose permission to use the fpu. This accelerates context
>  	 * switching for most programs since they don't use the fpu.
> @@ -124,7 +126,7 @@
>  	p->thread.cp0_status = read_c0_status() &
>                              ~(ST0_CU2|ST0_CU1|KU_MASK);
>  	childregs->cp0_status &= ~(ST0_CU2|ST0_CU1);
> -	p->user_tid = NULL;
> +	clear_ti_thread_flag(ti, TIF_USEDFPU);
>  
>  	return 0;
>  }
> 

Re: [PATCH 2.5] clear USEDFPU in copy_thread

[Juan Quintela]

>>>>> "jun" == Jun Sun <jsun@mvista.com> writes:

Hi

jun> Even if you don't have it cleared in start_thread(), things
jun> should be generally OK.  You will have some dirty FPU content
jun> instead of a all-zero one when you start a new program.  But then
jun> since all sane program should assign register values before they
jun> first time use them, so this bug should be well hidden.

I don't remind the exact details, but the problem appears to be the
security implications, you can see last values of previous process.

Yes, I still have to find a way where that is useful, but ...

Later, Juan.

-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy

Re: [PATCH 2.5] clear USEDFPU in copy_thread

[Jun Sun]

On Thu, Feb 06, 2003 at 04:43:42PM -0800, Jun Sun wrote:
> 
> You should *not* clear USEDFPU in copy_thread().  Imagine
> you assign a floating point variable f=0.3, then do fork()
> and then in the child process, alas, f is undefined (zero
> most likely).
> 
> If you really want to do it, it should be in start_thread().
>

This is plain stupid comment!  I was thinking about task->used_math
flag.  Please igore it.  
 
> I am still curious whether this is a bug in i386 as well or they do
> clear the flag in some non-obvious way.  Note that the unlazy_fpu()
> in copy_thread won't do it.  It only clears the current process's
> USEDFPU flag, while the child process's flag is set way back in
> copy_flags() calls inside do_fork().
>

But this comment still makes sense... 

Jun

Re: [PATCH 2.5] clear USEDFPU in copy_thread

[Vivien Chappelier]

> On Thu, Feb 06, 2003 at 04:43:42PM -0800, Jun Sun wrote:
> > I am still curious whether this is a bug in i386 as well or they do
> > clear the flag in some non-obvious way.  Note that the unlazy_fpu()
> > in copy_thread won't do it.  It only clears the current process's
> > USEDFPU flag, while the child process's flag is set way back in
> > copy_flags() calls inside do_fork().
> >

Well, I'm not an expert of linux/x86 at all, and this is a bit off
topic :), but look at the comment in arch/i386/process.c:408:
 * We fsave/fwait so that an exception goes off at the right time
 * (as a call from the fsave or fwait in effect) rather than to
 * the wrong process. Lazy FP saving no longer makes any sense
 * with modern CPU's, and this simplifies a lot of things (SMP
 * and UP become the same).

It seems they're just calling unlazy_fpu to do a fsave/fwait to
synchronize the FPU so that if an exception happens in the FPU code of a
process, the current process is signalled and not some newly scheduled
process instead. I think they do the same thing when cloning; put a
barrier to current process FPU operations so that it gets the signal, not
its child, if something is wrong in the previously executed FPU
instructions. Whether TIF_USEDFPU is set or cleared in the child isn't
really relevant in fact, it is maybe slightly inefficient because on
the first context switch of the child, an unnecessary fsave/fwait will be
done, but it doesn't hurt.

Vivien.