amd64/57250: Broken PTRACE_GETFPREGS and PTRACE_SETFPREGS

[Mark Kettenis]

>Number:         57250
>Category:       amd64
>Synopsis:       Broken PTRACE_GETFPREGS and PTRACE_SETFPREGS
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    freebsd-amd64
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Fri Sep 26 07:20:01 PDT 2003
>Closed-Date:
>Last-Modified:
>Originator:     Mark Kettenis
>Release:        FreeBSD 5.1-CURRENT amd64
>Organization:
>Environment:
System: FreeBSD delius.kettenis.dyndns.org 5.1-CURRENT FreeBSD 5.1-CURRENT #0: Sun Sep 21 23:09:00 CEST 2003 kettenis at delius.kettenis.dyndns.org:/usr/obj/usr/src/sys/GENERIC amd64

>Description:
The PTRACE_GETFPREGS and PTRACE_SETFPREGS requests are broken on
FreeBSD/amd64.  In particular, using PTRACE_SETFPREGS is pointless
until the program beging traced has executed its first floating-point
instruction; that floating-point instruction will reinitialize the
precess' floating-point state, overwriting the values set by
PTRACE_SETFPREGS.

I traced this back to src/sys/amd64/amd64/machdep.c:fill_fpregs()
where we read the floating-point state from the pcb, regardless of
whether PCB_NPXINITDONE has been set, and set_fpregs() where we don't
set PCB_NPXINITDONE, and fail to properly set the floating-point
registers if the current process "owns" the FPU.  This probably means
that /proc is broken too.

What puzzles me is that the amd64 code is pretty much identical to the
i386, and that everything appears to be working fine there, at least
with GDB.  The only thing I can think of is that every process
executes a floating-point instruction before reaching main().
However, I haven't been able to find such an instruction.  Anyway, I
think the code is incorrect on the i386 too, and that we just happen
not to notice where it matters.

>How-To-Repeat:

Using GDB, in a function that returns a floating-point value, type
"return 3.1415".  The caller of this function will see garbage instead
of this value.

>Fix:
The attached patch fixes things for me.  The functions
fill_fpregs_xmm() and set_fpregs_xmm() can probably be eliminated
passing fpregs directly to npxgetregs and npxsetregs.


--- src/sys/amd64/amd64/machdep.c.orig	Tue Sep  9 21:32:09 2003
+++ src/sys/amd64/amd64/machdep.c	Sat Sep 20 11:47:19 2003
@@ -1413,8 +1413,10 @@ set_fpregs_xmm(struct fpreg *fpregs, str
 int
 fill_fpregs(struct thread *td, struct fpreg *fpregs)
 {
+	struct savefpu sv_xmm;
 
-	fill_fpregs_xmm(&td->td_pcb->pcb_save, fpregs);
+  	npxgetregs(td, &sv_xmm);
+	fill_fpregs_xmm(&sv_xmm, fpregs);
 	return (0);
 }
 
@@ -1422,8 +1424,10 @@ fill_fpregs(struct thread *td, struct fp
 int
 set_fpregs(struct thread *td, struct fpreg *fpregs)
 {
+	struct savefpu sv_xmm;
 
-	set_fpregs_xmm(fpregs, &td->td_pcb->pcb_save);
+	set_fpregs_xmm(fpregs, &sv_xmm);
+  	npxsetregs(td, &sv_xmm);
 	return (0);
 }
 
--- src/sys/amd64/isa/npx.c.orig	Fri Jul 25 23:18:52 2003
+++ src/sys/amd64/isa/npx.c	Sat Sep 20 11:47:19 2003
@@ -646,7 +646,7 @@ npxsetregs(td, addr)
 		intr_restore(s);
 		bcopy(addr, &td->td_pcb->pcb_save, sizeof(*addr));
 	}
-	curthread->td_pcb->pcb_flags |= PCB_NPXINITDONE;
+	td->td_pcb->pcb_flags |= PCB_NPXINITDONE;
 }
 
 static device_method_t npx_methods[] = {



>Release-Note:
>Audit-Trail:
>Unformatted: