--- a/arch/x86/kernel/i387.c 2008-04-17 11:49:44.000000000 +0900
+++ b/arch/x86/kernel/i387.c 2008-06-30 13:22:57.000000000 +0900
@@ -130,7 +130,7 @@
void *kbuf, void __user *ubuf)
{
if (!cpu_has_fxsr)
- return -ENODEV;
+ return -EIO;
init_fpu(target);
@@ -145,7 +145,7 @@
int ret;
if (!cpu_has_fxsr)
- return -ENODEV;
+ return -EIO;
init_fpu(target);
set_stopped_child_used_math(target);
* TAKADA Yoshihito <[email protected]> wrote:
> Hi
> When I update kernel 2.6.25 from 2.6.24, gdb does not work.
> On 2.6.25, ptrace(PTRACE_GETFPXREGS, ...) returns ENODEV.
> But 2.6.24 kernel's ptrace() returns EIO.
> It is issue of compatibility.
> I attached test program as pt.c and patch for fix it.
this is a side-effect of the "x86: x86 i387 user_regset" commit,
v2.6.24-4379-g4421011. Roland Cc:-ed.
Ingo
Thanks for the report. That was indeed a regression from 2.6.24 in ptrace.
To match traditional behavior, ptrace should only ever return -EIO for all
kinds of errors accessing registers (except for the -ESRCH cases). But it
is cleaner for the user_regset calls to use meaningful error codes, and
-ENODEV is what makes sense for the !cpu_has_fxsr case.
So the right fix for this is in ptrace, not in user_regset.
Ingo, please revert takada's patch and apply the one about to come from me.
Thanks,
Roland
ptrace has always returned only -EIO for all failures to access
registers. The user_regset calls are allowed to return a more
meaningful variety of errors. The REGSET_XFP calls use -ENODEV
for !cpu_has_fxsr hardware. Make ptrace return the traditional
-EIO instead of the error code from the user_regset call.
Signed-off-by: Roland McGrath <[email protected]>
---
arch/x86/kernel/ptrace.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index a7835f2..77040b6 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -943,13 +943,13 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
return copy_regset_to_user(child, &user_x86_32_view,
REGSET_XFP,
0, sizeof(struct user_fxsr_struct),
- datap);
+ datap) ? -EIO : 0;
case PTRACE_SETFPXREGS: /* Set the child extended FPU state. */
return copy_regset_from_user(child, &user_x86_32_view,
REGSET_XFP,
0, sizeof(struct user_fxsr_struct),
- datap);
+ datap) ? -EIO : 0;
#endif
#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
* Roland McGrath <[email protected]> wrote:
> ptrace has always returned only -EIO for all failures to access
> registers. The user_regset calls are allowed to return a more
> meaningful variety of errors. The REGSET_XFP calls use -ENODEV for
> !cpu_has_fxsr hardware. Make ptrace return the traditional -EIO
> instead of the error code from the user_regset call.
since the original fix is already upstream, i've applied the delta patch
below. Should we still do this for v2.6.26 or can we defer it to
v2.6.27? As ptrace is the only user of this facility for now this would
be an identity transformation AFAICS and the v2.6.26 release is very
close.
Ingo
---------------->
Subject: x86 ptrace: fix PTRACE_GETFPXREGS error
From: Roland McGrath <[email protected]>
Date: Mon, 30 Jun 2008 14:02:41 -0700 (PDT)
ptrace has always returned only -EIO for all failures to access
registers. The user_regset calls are allowed to return a more
meaningful variety of errors. The REGSET_XFP calls use -ENODEV
for !cpu_has_fxsr hardware. Make ptrace return the traditional
-EIO instead of the error code from the user_regset call.
Signed-off-by: Roland McGrath <[email protected]>
Cc: [email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/i387.c | 4 ++--
arch/x86/kernel/ptrace.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
Index: tip/arch/x86/kernel/i387.c
===================================================================
--- tip.orig/arch/x86/kernel/i387.c
+++ tip/arch/x86/kernel/i387.c
@@ -162,7 +162,7 @@ int xfpregs_get(struct task_struct *targ
int ret;
if (!cpu_has_fxsr)
- return -EIO;
+ return -ENODEV;
ret = init_fpu(target);
if (ret)
@@ -179,7 +179,7 @@ int xfpregs_set(struct task_struct *targ
int ret;
if (!cpu_has_fxsr)
- return -EIO;
+ return -ENODEV;
ret = init_fpu(target);
if (ret)
Index: tip/arch/x86/kernel/ptrace.c
===================================================================
--- tip.orig/arch/x86/kernel/ptrace.c
+++ tip/arch/x86/kernel/ptrace.c
@@ -943,13 +943,13 @@ long arch_ptrace(struct task_struct *chi
return copy_regset_to_user(child, &user_x86_32_view,
REGSET_XFP,
0, sizeof(struct user_fxsr_struct),
- datap);
+ datap) ? -EIO : 0;
case PTRACE_SETFPXREGS: /* Set the child extended FPU state. */
return copy_regset_from_user(child, &user_x86_32_view,
REGSET_XFP,
0, sizeof(struct user_fxsr_struct),
- datap);
+ datap) ? -EIO : 0;
#endif
#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
> since the original fix is already upstream, i've applied the delta patch
> below. Should we still do this for v2.6.26 or can we defer it to
> v2.6.27? As ptrace is the only user of this facility for now this would
> be an identity transformation AFAICS and the v2.6.26 release is very
> close.
I don't think there's a problem with 2.6.26 either way. I agree that the
user_regset internal API does not matter much before 2.6.27.
My patch alone applies to 2.6.25, which is why I CC'd it to stable.
I think applying that (and not takada's patch) to stable-2.6.25
would be best.
Thanks,
Roland
* Roland McGrath <[email protected]> wrote:
> > since the original fix is already upstream, i've applied the delta
> > patch below. Should we still do this for v2.6.26 or can we defer it
> > to v2.6.27? As ptrace is the only user of this facility for now this
> > would be an identity transformation AFAICS and the v2.6.26 release
> > is very close.
>
> I don't think there's a problem with 2.6.26 either way. I agree that
> the user_regset internal API does not matter much before 2.6.27.
okay - i've queued it up in tip/x86/ptrace for now.
> My patch alone applies to 2.6.25, which is why I CC'd it to stable. I
> think applying that (and not takada's patch) to stable-2.6.25 would be
> best.
i think Greg already queued the original fix up for v2.6.25, as per the
commit notifier below.
so i think it is all sorted fine now?
Ingo
---------------------->
This is a note to let you know that we have just queued up the patch titled
Subject: ptrace GET/SET FPXREGS broken
to the 2.6.25-stable tree. Its filename is
ptrace-get-set-fpxregs-broken.patch
A git repo of this tree can be found at
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>From [email protected] Mon Jun 30 09:22:46 2008
From: TAKADA Yoshihito <[email protected]>
Date: Mon, 30 Jun 2008 18:22:07 +0200
Subject: ptrace GET/SET FPXREGS broken
To: [email protected]
Message-ID: <[email protected]>
Content-Disposition: inline
From: TAKADA Yoshihito <[email protected]>
Commit 11dbc963a8f6128595d0f6ecf138dc369e144997 upstream
ptrace GET/SET FPXREGS broken
When I update kernel 2.6.25 from 2.6.24, gdb does not work.
On 2.6.25, ptrace(PTRACE_GETFPXREGS, ...) returns ENODEV.
But 2.6.24 kernel's ptrace() returns EIO.
It is issue of compatibility.
I attached test program as pt.c and patch for fix it.
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <signal.h>
#include <errno.h>
#include <sys/ptrace.h>
#include <sys/types.h>
struct user_fxsr_struct {
unsigned short cwd;
unsigned short swd;
unsigned short twd;
unsigned short fop;
long fip;
long fcs;
long foo;
long fos;
long mxcsr;
long reserved;
long st_space[32]; /* 8*16 bytes for each FP-reg = 128 bytes */
long xmm_space[32]; /* 8*16 bytes for each XMM-reg = 128 bytes */
long padding[56];
};
int main(void)
{
pid_t pid;
pid = fork();
switch(pid){
case -1:/* error */
break;
case 0:/* child */
child();
break;
default:
parent(pid);
break;
}
return 0;
}
int child(void)
{
ptrace(PTRACE_TRACEME);
kill(getpid(), SIGSTOP);
sleep(10);
return 0;
}
int parent(pid_t pid)
{
int ret;
struct user_fxsr_struct fpxregs;
ret = ptrace(PTRACE_GETFPXREGS, pid, 0, &fpxregs);
if(ret < 0){
printf("%d: %s.\n", errno, strerror(errno));
}
kill(pid, SIGCONT);
wait(pid);
return 0;
}
/* in the kerel, at kernel/i387.c get_fpxregs() */
Signed-off-by: Ingo Molnar <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
arch/x86/kernel/i387.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -130,7 +130,7 @@ int xfpregs_get(struct task_struct *targ
void *kbuf, void __user *ubuf)
{
if (!cpu_has_fxsr)
- return -ENODEV;
+ return -EIO;
init_fpu(target);
@@ -145,7 +145,7 @@ int xfpregs_set(struct task_struct *targ
int ret;
if (!cpu_has_fxsr)
- return -ENODEV;
+ return -EIO;
init_fpu(target);
set_stopped_child_used_math(target);
Patches currently in stable-queue which might be from [email protected] are
queue-2.6.25/ptrace-get-set-fpxregs-broken.patch
On Tue, Jul 01, 2008 at 12:11:04PM +0200, Ingo Molnar wrote:
>
> * Roland McGrath <[email protected]> wrote:
>
> > > since the original fix is already upstream, i've applied the delta
> > > patch below. Should we still do this for v2.6.26 or can we defer it
> > > to v2.6.27? As ptrace is the only user of this facility for now this
> > > would be an identity transformation AFAICS and the v2.6.26 release
> > > is very close.
> >
> > I don't think there's a problem with 2.6.26 either way. I agree that
> > the user_regset internal API does not matter much before 2.6.27.
>
> okay - i've queued it up in tip/x86/ptrace for now.
>
> > My patch alone applies to 2.6.25, which is why I CC'd it to stable. I
> > think applying that (and not takada's patch) to stable-2.6.25 would be
> > best.
>
> i think Greg already queued the original fix up for v2.6.25, as per the
> commit notifier below.
Yes I did queue that one up, but it looks different from Roland's
original patch in this thread.
Roland, does the patch below suffice? Or should I take your original
one instead, or as well?
thanks,
greg k-h
* Greg KH <[email protected]> wrote:
> On Tue, Jul 01, 2008 at 12:11:04PM +0200, Ingo Molnar wrote:
> >
> > * Roland McGrath <[email protected]> wrote:
> >
> > > > since the original fix is already upstream, i've applied the delta
> > > > patch below. Should we still do this for v2.6.26 or can we defer it
> > > > to v2.6.27? As ptrace is the only user of this facility for now this
> > > > would be an identity transformation AFAICS and the v2.6.26 release
> > > > is very close.
> > >
> > > I don't think there's a problem with 2.6.26 either way. I agree that
> > > the user_regset internal API does not matter much before 2.6.27.
> >
> > okay - i've queued it up in tip/x86/ptrace for now.
> >
> > > My patch alone applies to 2.6.25, which is why I CC'd it to stable. I
> > > think applying that (and not takada's patch) to stable-2.6.25 would be
> > > best.
> >
> > i think Greg already queued the original fix up for v2.6.25, as per the
> > commit notifier below.
>
> Yes I did queue that one up, but it looks different from Roland's
> original patch in this thread.
yes, but as discussed in this thread, both patches are fine in principle
as far as the regression goes.
Ingo
On Tue, Jul 01, 2008 at 04:46:58PM +0200, Ingo Molnar wrote:
>
> * Greg KH <[email protected]> wrote:
>
> > On Tue, Jul 01, 2008 at 12:11:04PM +0200, Ingo Molnar wrote:
> > >
> > > * Roland McGrath <[email protected]> wrote:
> > >
> > > > > since the original fix is already upstream, i've applied the delta
> > > > > patch below. Should we still do this for v2.6.26 or can we defer it
> > > > > to v2.6.27? As ptrace is the only user of this facility for now this
> > > > > would be an identity transformation AFAICS and the v2.6.26 release
> > > > > is very close.
> > > >
> > > > I don't think there's a problem with 2.6.26 either way. I agree that
> > > > the user_regset internal API does not matter much before 2.6.27.
> > >
> > > okay - i've queued it up in tip/x86/ptrace for now.
> > >
> > > > My patch alone applies to 2.6.25, which is why I CC'd it to stable. I
> > > > think applying that (and not takada's patch) to stable-2.6.25 would be
> > > > best.
> > >
> > > i think Greg already queued the original fix up for v2.6.25, as per the
> > > commit notifier below.
> >
> > Yes I did queue that one up, but it looks different from Roland's
> > original patch in this thread.
>
> yes, but as discussed in this thread, both patches are fine in principle
> as far as the regression goes.
Ah, great, sorry I missed that. I'll leave the current patch in our
tree and go get more coffee :)
thanks,
greg k-h
Hi. Thanks for your right patch.
BTW, are FXSAVE and FXRSTOR instructions device?
Is it right to return ENODEV?
I think I had bettor return EXIO or ENOTSUP.
If it discussed, tell me URL of tree at lkml.org.
From: Roland McGrath <[email protected]>
Subject: [PATCH] x86 ptrace: fix PTRACE_GETFPXREGS error
Date: Mon, 30 Jun 2008 14:02:41 -0700 (PDT)
> ptrace has always returned only -EIO for all failures to access
> registers. The user_regset calls are allowed to return a more
> meaningful variety of errors. The REGSET_XFP calls use -ENODEV
> for !cpu_has_fxsr hardware. Make ptrace return the traditional
> -EIO instead of the error code from the user_regset call.
>
> Signed-off-by: Roland McGrath <[email protected]>
> ---
> arch/x86/kernel/ptrace.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index a7835f2..77040b6 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -943,13 +943,13 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
> return copy_regset_to_user(child, &user_x86_32_view,
> REGSET_XFP,
> 0, sizeof(struct user_fxsr_struct),
> - datap);
> + datap) ? -EIO : 0;
>
> case PTRACE_SETFPXREGS: /* Set the child extended FPU state. */
> return copy_regset_from_user(child, &user_x86_32_view,
> REGSET_XFP,
> 0, sizeof(struct user_fxsr_struct),
> - datap);
> + datap) ? -EIO : 0;
> #endif
>
> #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hi.
The Roland's patch is bettor than mine.
Please apply Roland's only. Don't apply both. Either is enough.
From: Ingo Molnar <[email protected]>
Subject: Re: [PATCH] x86 ptrace: fix PTRACE_GETFPXREGS error
Date: Tue, 1 Jul 2008 11:02:04 +0200
>
> * Roland McGrath <[email protected]> wrote:
>
> > ptrace has always returned only -EIO for all failures to access
> > registers. The user_regset calls are allowed to return a more
> > meaningful variety of errors. The REGSET_XFP calls use -ENODEV for
> > !cpu_has_fxsr hardware. Make ptrace return the traditional -EIO
> > instead of the error code from the user_regset call.
>
> since the original fix is already upstream, i've applied the delta patch
> below. Should we still do this for v2.6.26 or can we defer it to
> v2.6.27? As ptrace is the only user of this facility for now this would
> be an identity transformation AFAICS and the v2.6.26 release is very
> close.
>
> Ingo
>
> ---------------->
> Subject: x86 ptrace: fix PTRACE_GETFPXREGS error
> From: Roland McGrath <[email protected]>
> Date: Mon, 30 Jun 2008 14:02:41 -0700 (PDT)
>
> ptrace has always returned only -EIO for all failures to access
> registers. The user_regset calls are allowed to return a more
> meaningful variety of errors. The REGSET_XFP calls use -ENODEV
> for !cpu_has_fxsr hardware. Make ptrace return the traditional
> -EIO instead of the error code from the user_regset call.
>
> Signed-off-by: Roland McGrath <[email protected]>
> Cc: [email protected]
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> arch/x86/kernel/i387.c | 4 ++--
> arch/x86/kernel/ptrace.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> Index: tip/arch/x86/kernel/i387.c
> ===================================================================
> --- tip.orig/arch/x86/kernel/i387.c
> +++ tip/arch/x86/kernel/i387.c
> @@ -162,7 +162,7 @@ int xfpregs_get(struct task_struct *targ
> int ret;
>
> if (!cpu_has_fxsr)
> - return -EIO;
> + return -ENODEV;
>
> ret = init_fpu(target);
> if (ret)
> @@ -179,7 +179,7 @@ int xfpregs_set(struct task_struct *targ
> int ret;
>
> if (!cpu_has_fxsr)
> - return -EIO;
> + return -ENODEV;
>
> ret = init_fpu(target);
> if (ret)
> Index: tip/arch/x86/kernel/ptrace.c
> ===================================================================
> --- tip.orig/arch/x86/kernel/ptrace.c
> +++ tip/arch/x86/kernel/ptrace.c
> @@ -943,13 +943,13 @@ long arch_ptrace(struct task_struct *chi
> return copy_regset_to_user(child, &user_x86_32_view,
> REGSET_XFP,
> 0, sizeof(struct user_fxsr_struct),
> - datap);
> + datap) ? -EIO : 0;
>
> case PTRACE_SETFPXREGS: /* Set the child extended FPU state. */
> return copy_regset_from_user(child, &user_x86_32_view,
> REGSET_XFP,
> 0, sizeof(struct user_fxsr_struct),
> - datap);
> + datap) ? -EIO : 0;
> #endif
>
> #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
> Hi. Thanks for your right patch.
> BTW, are FXSAVE and FXRSTOR instructions device?
> Is it right to return ENODEV?
> I think I had bettor return EXIO or ENOTSUP.
> If it discussed, tell me URL of tree at lkml.org.
I don't think it was discussed. It's clearly documented in the
linux/regset.h kerneldoc comments that define the interface.
I don't recall any feedback about error codes when that went in.
Device, eh, whatever. We're not going to add a new code that means
precisely "this user_regset refers to hardware not present", that would be
silly. It's going to be chosen by some analogy with the original use of
some existing code. ENXIO is fine too. I probably chose ENODEV over ENXIO
just because it's easier to read the name and remember what it might mean
(and it's easier to type).
All that really matters is that for the particular case of user_regset
functions, there be clearly specified one errno code that means "all's well,
but this supported hardware is not here". Then callers know to check for
that, and any other error code value is potentially an unexpected sort of
error. I see nothing wrong with ENODEV. The only thing wrong with ENXIO is
trying to type it correctly twice in a row. ENOTSUP does not seem like a
good fit as an analogy to its other specified uses.
Thanks,
Roland