Bug 57250

Summary: [amd64] [patch] Broken PTRACE_GETFPREGS and PTRACE_SETFPREGS
Product: Base System Reporter: Mark Kettenis <kettenis>
Component: amd64Assignee: freebsd-amd64 (Nobody) <amd64>
Status: Closed FIXED    
Severity: Affects Only Me    
Priority: Normal    
Version: 5.1-CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
file.diff none

Description Mark Kettenis 2003-09-26 15:20:01 UTC 
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.

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.

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.
Comment 1 David E. O'Brien freebsd_committer freebsd_triage 2003-10-28 20:05:52 UTC 
Responsible Changed
From-To: freebsd-amd64->obrien

I'll take this one.
Comment 2 Mark Kettenis 2003-10-30 20:01:07 UTC 
The patch I submitted is broken.  The GDB testsuite has gained a new
test which makes my kernel (which includes the patch) panic:

kernel trap with interrupts disabled

npxgetregs+0x5c

Mark
Comment 3 Mark Kettenis 2003-12-28 21:21:17 UTC 
Here's a new patch.  It fixes the problem for me and doesn't seem to
cause any ill effects.  It does things a little different than my
earlier patch; when setting the FP registers I now simply drop the
FPU, and copy the new register state into the pcb.  That way I avoid
generating floating point exceptions with interrupts disabled.  Any
pending exceptions will be generated when the program continues and
loads the FPU upon execution of its first FP instruction.  This seems
the right approach for setcontext() too.

Mark

--- src/sys/amd64/amd64/fpu.c.orig	Mon Dec 15 18:54:23 2003
+++ src/sys/amd64/amd64/fpu.c	Sat Dec 20 22:28:16 2003
@@ -510,13 +510,12 @@ fpusetregs(struct thread *td, struct sav
 
 	s = intr_disable();
 	if (td == PCPU_GET(fpcurthread)) {
-		fxrstor(addr);
-		intr_restore(s);
-	} else {
-		intr_restore(s);
-		bcopy(addr, &td->td_pcb->pcb_save, sizeof(*addr));
+		PCPU_SET(fpcurthread, NULL);
+		start_emulating();
 	}
-	curthread->td_pcb->pcb_flags |= PCB_FPUINITDONE;
+	intr_restore(s);
+	bcopy(addr, &td->td_pcb->pcb_save, sizeof(*addr));
+	td->td_pcb->pcb_flags |= PCB_FPUINITDONE;
 }
 
 /*
--- src/sys/amd64/amd64/machdep.c.orig	Sun Dec  7 00:19:47 2003
+++ src/sys/amd64/amd64/machdep.c	Sat Dec 20 23:09:02 2003
@@ -1403,8 +1403,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);
+	fpugetregs(td, &sv_xmm);
+	fill_fpregs_xmm(&sv_xmm, fpregs);
 	return (0);
 }
 
@@ -1412,8 +1414,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);
+	fpusetregs(td, &sv_xmm);
 	return (0);
 }
Comment 4 K. Macy freebsd_committer freebsd_triage 2007-11-16 22:42:16 UTC 
State Changed
From-To: open->feedback


Am I correct in assuming that this is still an issue?
Comment 5 K. Macy freebsd_committer freebsd_triage 2007-11-16 22:42:16 UTC 
Responsible Changed
From-To: obrien->kmacy


Am I correct in assuming that this is still an issue?
Comment 6 Mark Linimon freebsd_committer freebsd_triage 2008-03-01 20:14:06 UTC 
State Changed
From-To: feedback->suspended

No feedback received, but it sounds like this could still be an issue.
Comment 7 Andriy Gapon freebsd_committer freebsd_triage 2010-12-05 13:37:09 UTC 
Responsible Changed
From-To: kmacy->freebsd-amd64

No activity for quite a while, release back to the pool.
Comment 8 Andriy Gapon freebsd_committer freebsd_triage 2010-12-05 13:38:12 UTC 
Kostik,

could you please assess the status of this issue?
Thanks!
-- 
Andriy Gapon
Comment 9 Kostik Belousov 2010-12-05 14:11:36 UTC 
On Sun, Dec 05, 2010 at 03:38:12PM +0200, Andriy Gapon wrote:
> 
> Kostik,
> 
> could you please assess the status of this issue?
> Thanks!
> -- 
> Andriy Gapon

Amuzingly enough, I fixed this in r215865 on 2010-11-26, that was
merged to stable/8 in r216162 on 2010-12-03, even forgetting mentioning
this in the commit message.
Comment 10 Andriy Gapon freebsd_committer freebsd_triage 2010-12-05 14:26:30 UTC 
on 05/12/2010 16:11 Kostik Belousov said the following:
> On Sun, Dec 05, 2010 at 03:38:12PM +0200, Andriy Gapon wrote:
>>
>> Kostik,
>>
>> could you please assess the status of this issue?
>> Thanks!
>> -- 
>> Andriy Gapon
> Amuzingly enough, I fixed this in r215865 on 2010-11-26, that was
> merged to stable/8 in r216162 on 2010-12-03, even forgetting mentioning
> this in the commit message.

I suspected something like that :-)
Thanks a lot!

-- 
Andriy Gapon
Comment 11 Andriy Gapon freebsd_committer freebsd_triage 2010-12-05 14:26:46 UTC 
State Changed
From-To: suspended->closed

This should be resolved now in head and stable/8.