This adds the forced_successful_syscall_return() macro, a mate to
force_successful_syscall_return() to test rather than set the condition.
Signed-off-by: Roland McGrath <[email protected]>
---
include/linux/ptrace.h | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index ebe0c17..9272e63 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -124,8 +124,13 @@ int generic_ptrace_pokedata(struct task_struct *tsk, long addr, long data);
* is a no-op and the spurious error condition needs to be filtered out by some
* other means (e.g., in user-level, by passing an extra argument to the
* syscall handler, or something along those lines).
+ *
+ * On architectures that define force_successful_syscall_return(),
+ * forced_successful_syscall_return() should also be defined.
+ * It returns nonzero if force_successful_syscall_return() was just used.
*/
#define force_successful_syscall_return() do { } while (0)
+#define forced_successful_syscall_return() 0
#endif
/*
Define the forced_successful_syscall_return() macro to
pair with our force_successful_syscall_return() definition.
Signed-off-by: Roland McGrath <[email protected]>
---
include/asm-alpha/ptrace.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/asm-alpha/ptrace.h b/include/asm-alpha/ptrace.h
index 32c7a5c..1cc330b 100644
--- a/include/asm-alpha/ptrace.h
+++ b/include/asm-alpha/ptrace.h
@@ -77,6 +77,7 @@ extern void show_regs(struct pt_regs *);
((struct pt_regs *) (task_stack_page(task) + 2*PAGE_SIZE) - 1)
#define force_successful_syscall_return() (task_pt_regs(current)->r0 = 0)
+#define forced_successful_syscall_return() (task_pt_regs(current)->r0 == 0)
#endif
Define the forced_successful_syscall_return() macro to
pair with our force_successful_syscall_return() definition.
Signed-off-by: Roland McGrath <[email protected]>
---
include/asm-powerpc/ptrace.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/asm-powerpc/ptrace.h b/include/asm-powerpc/ptrace.h
index 891d689..07dd6fe 100644
--- a/include/asm-powerpc/ptrace.h
+++ b/include/asm-powerpc/ptrace.h
@@ -93,6 +93,7 @@ extern unsigned long profile_pc(struct pt_regs *regs);
do { \
set_thread_flag(TIF_NOERROR); \
} while(0)
+#define forced_successful_syscall_return() (test_thread_flag(TIF_NOERROR))
struct task_struct;
extern unsigned long ptrace_get_reg(struct task_struct *task, int regno);
Define the forced_successful_syscall_return() macro to
pair with our force_successful_syscall_return() definition.
Signed-off-by: Roland McGrath <[email protected]>
---
include/asm-ia64/ptrace.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/asm-ia64/ptrace.h b/include/asm-ia64/ptrace.h
index 4b2a8d4..5e82915 100644
--- a/include/asm-ia64/ptrace.h
+++ b/include/asm-ia64/ptrace.h
@@ -276,6 +276,7 @@ struct switch_stack {
* On ia64, we can clear the user's pt_regs->r8 to force a successful syscall.
*/
# define force_successful_syscall_return() (task_pt_regs(current)->r8 = 0)
+# define forced_successful_syscall_return() (task_pt_regs(current)->r8 == 0)
struct task_struct; /* forward decl */
struct unw_frame_info; /* forward decl */
Define the forced_successful_syscall_return() macro to
pair with our force_successful_syscall_return() definition.
Signed-off-by: Roland McGrath <[email protected]>
---
include/asm-sparc64/ptrace.h | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/include/asm-sparc64/ptrace.h b/include/asm-sparc64/ptrace.h
index 6da1978..3f8826e 100644
--- a/include/asm-sparc64/ptrace.h
+++ b/include/asm-sparc64/ptrace.h
@@ -100,6 +100,8 @@ struct sparc_trapf {
#define force_successful_syscall_return() \
do { current_thread_info()->syscall_noerror = 1; \
} while (0)
+#define forced_successful_syscall_return() \
+ (current_thread_info()->syscall_noerror != 0)
#define user_mode(regs) (!((regs)->tstate & TSTATE_PRIV))
#define instruction_pointer(regs) ((regs)->tpc)
#define regs_return_value(regs) ((regs)->u_regs[UREG_I0])
The arch_ptrace and compat_arch_ptrace functions can now return
-ENOSYS for requests they do not actually implement in arch
code. Returning -ENOSYS replaces the calls to ptrace_request
and compat_ptrace_request. This leaves more latitude for the
machine-independent ptrace implementation code to change without
requiring any more updates in the arch code.
This change has no effect on the old arch code that still calls
ptrace_request or compat_ptrace_request.
Signed-off-by: Roland McGrath <[email protected]>
---
kernel/ptrace.c | 42 ++++++++++++++++++++++++++----------------
1 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 67e392e..60b2b57 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -574,18 +574,21 @@ asmlinkage long sys_ptrace(long request, long pid, long addr, long data)
*/
if (!ret)
arch_ptrace_attach(child);
- goto out_put_task_struct;
+ } else {
+ ret = ptrace_check_attach(child, request == PTRACE_KILL);
+ /*
+ * Let the arch handler inspect it first. It returns
+ * -ENOSYS if this is not an arch-specific request.
+ */
+ if (!ret) {
+ ret = arch_ptrace(child, request, addr, data);
+ if (ret == -ENOSYS &&
+ !forced_successful_syscall_return())
+ ret = ptrace_request(child, request,
+ addr, data);
+ }
}
- ret = ptrace_check_attach(child, request == PTRACE_KILL);
- if (ret < 0)
- goto out_put_task_struct;
-
- ret = arch_ptrace(child, request, addr, data);
- if (ret < 0)
- goto out_put_task_struct;
-
- out_put_task_struct:
put_task_struct(child);
out:
unlock_kernel();
@@ -697,14 +700,21 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
*/
if (!ret)
arch_ptrace_attach(child);
- goto out_put_task_struct;
+ } else {
+ ret = ptrace_check_attach(child, request == PTRACE_KILL);
+ /*
+ * Let the arch handler inspect it first. It returns
+ * -ENOSYS if this is not an arch-specific request.
+ */
+ if (!ret) {
+ ret = compat_arch_ptrace(child, request, addr, data);
+ if (ret == -ENOSYS &&
+ !forced_successful_syscall_return())
+ ret = compat_ptrace_request(child, request,
+ addr, data);
+ }
}
- ret = ptrace_check_attach(child, request == PTRACE_KILL);
- if (!ret)
- ret = compat_arch_ptrace(child, request, addr, data);
-
- out_put_task_struct:
put_task_struct(child);
out:
unlock_kernel();
Return -ENOSYS from arch_ptrace and compat_arch_ptrace
rather than calling ptrace_request or compat_ptrace_request.
Signed-off-by: Roland McGrath <[email protected]>
---
arch/x86/kernel/ptrace.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 777d8f9..4d5561c 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1010,7 +1010,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
#endif
default:
- ret = ptrace_request(child, request, addr, data);
+ ret = -ENOSYS;
break;
}
@@ -1266,7 +1266,8 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
return arch_ptrace(child, request, addr, data);
default:
- return compat_ptrace_request(child, request, addr, data);
+ ret = -ENOSYS;
+ break;
}
return ret;
Return -ENOSYS from arch_ptrace and compat_arch_ptrace
rather than calling ptrace_request or compat_ptrace_request.
Signed-off-by: Roland McGrath <[email protected]>
---
arch/powerpc/kernel/ptrace.c | 2 +-
arch/powerpc/kernel/ptrace32.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 2a9fe97..47b701e 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -843,7 +843,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
break;
default:
- ret = ptrace_request(child, request, addr, data);
+ ret = -ENOSYS;
break;
}
return ret;
diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
index 4c1de6a..7383d15 100644
--- a/arch/powerpc/kernel/ptrace32.c
+++ b/arch/powerpc/kernel/ptrace32.c
@@ -306,7 +306,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
break;
default:
- ret = compat_ptrace_request(child, request, addr, data);
+ ret = -ENOSYS;
break;
}
From: Roland McGrath <[email protected]>
Date: Wed, 19 Mar 2008 14:20:01 -0700 (PDT)
> Define the forced_successful_syscall_return() macro to
> pair with our force_successful_syscall_return() definition.
>
> Signed-off-by: Roland McGrath <[email protected]>
Acked-by: David S. Miller <[email protected]>
On Wed, 19 Mar 2008, Roland McGrath wrote:
>
> The arch_ptrace and compat_arch_ptrace functions can now return
> -ENOSYS for requests they do not actually implement in arch
> code.
Hmm.. I see the whole series, and I see this patch, but I think it adds
new code and new complexity, and I don't really see *why*.
So I'm obviously not going to apply it outside the merge window anyway,
but even for later I'd really like to know what you're building up
towards, because without understanding the upsides it just feels like it
adds ugly code and unnecessary infrastructure without any real point to
it.
And I have to say, I really hate that
ret = arch_ptrace(child, request, addr, data);
if (ret == -ENOSYS && !forced_successful_syscall_return())
ret = ptrace_request(child, request, addr, data);
thing. Instead of doing it that ugly way (return value and a special
per-arch forced_successful_syscall_return() thing), this really smells
like you just want to change the calling conventions for "arch_ptrace()"
instead.
Wouldn't it be nicer to just let "arch_ptrace()" return a flag saying
whether it handled things or not?
Linus
On Wed, Mar 19, 2008 at 07:40:25PM -0700, Linus Torvalds wrote:
>
> And I have to say, I really hate that
>
> ret = arch_ptrace(child, request, addr, data);
> if (ret == -ENOSYS && !forced_successful_syscall_return())
> ret = ptrace_request(child, request, addr, data);
>
> thing. Instead of doing it that ugly way (return value and a special
> per-arch forced_successful_syscall_return() thing), this really smells
> like you just want to change the calling conventions for "arch_ptrace()"
> instead.
>
> Wouldn't it be nicer to just let "arch_ptrace()" return a flag saying
> whether it handled things or not?
I think the easiest and cleanest would be to just drop this whole
series. There's no inherent advantage of
ret = -ENOSYS;
in the arch_ptrace default case over
ret = ptrace_request(...);
> Hmm.. I see the whole series, and I see this patch, but I think it adds
> new code and new complexity, and I don't really see *why*.
The motivation is to get the arch function out of the code path for the
machine-independent request handling. I want to be able to change the
implementation later without touching the arch code again.
The arguments passed down to arch_ptrace are sufficient for what the arch
code itself needs and for the current implementation in ptrace_request.
In future, I'd like the option of changing the code for the standard
requests to use a local data structure set up at the start of ptrace, and
such like (so more pointers and the like would need to be passed down to
ptrace_request). These patches let me remove ptrace_request or change
its calling convention without touching the arch code again.
> Wouldn't it be nicer to just let "arch_ptrace()" return a flag saying
> whether it handled things or not?
It would certainly be nicer. I would prefer:
extern int arch_ptrace(struct task_struct *child, long request,
long addr, long data, long *retval);
where it returns an error code or it returns 0 and *retval is the value
or it returns 1 and it didn't do anything.
The reason I took the approach I did instead is incrementalism.
I can't change that signature without breaking about 22 arch builds.
I'm only really prepared to thoroughly verify a change on 2 of those
myself. It should be a simple enough change to make blind and get
right. But I've gotten a lot of things wrong before. On principle,
I wouldn't really expect anyone to sign off on stuff I won't even
claim to have tried. I did the forced_successful_syscall_return()
macro for arch's I don't try to build, and was just awful sure golly
that I hadn't got them wrong, because the generic change would break
those few arch's (not 20) without it.
So this ugliness seemed like a better bet than waiting for 20 more
arch sign-offs before any of it could go in. You are certainly in a
position to just change the generic signature and make every arch do
the update (or fix your typos if you just tweak them all blind), and
let them grumble. I did not presume to do so.
If you'd like a patch that changes this signature, updates all arch
implementations, and is actually verified to compile and work only
on x86 and powerpc, I'll be happy to provide that.
Thanks,
Roland
On Thu, 20 Mar 2008, Roland McGrath wrote:
> > Wouldn't it be nicer to just let "arch_ptrace()" return a flag saying
> > whether it handled things or not?
>
> It would certainly be nicer. I would prefer:
>
> extern int arch_ptrace(struct task_struct *child, long request,
> long addr, long data, long *retval);
>
> where it returns an error code or it returns 0 and *retval is the value
> or it returns 1 and it didn't do anything.
>
> So this ugliness seemed like a better bet than waiting for 20 more
> arch sign-offs before any of it could go in. You are certainly in a
> position to just change the generic signature and make every arch do
> the update (or fix your typos if you just tweak them all blind), and
> let them grumble. I did not presume to do so.
What about adding a CONFIG_ARCH_HAS_PTRACE2, which is set by the archs
which are converted. For those which are not you add a fallback
implementation:
#ifndef CONFIG_ARCH_HAS_PTRACE2
static int arch_ptrace2(whatever your favourite interface)
{
ret = arch_ptrace();
return do_ugly_fixups(ret);
}
#endif
That way you introduce the new interface and convert one or two archs
initialy without breaking the other 22.
At the same time you mark arch_ptrace() deprecated so it will get the
attention of the arch maintainers pretty fast. Once all archs are
converted we can remove the config flag and the fallback quirk.
Thanks,
tglx
On Fri, Mar 21, 2008 at 02:50:01PM +0100, Thomas Gleixner wrote:
> What about adding a CONFIG_ARCH_HAS_PTRACE2, which is set by the archs
> which are converted. For those which are not you add a fallback
> implementation:
Bah. Folks, we're talking about adding a single new argument to a
single function implemented by our 24 architectures. This is a trivial
almost scriptable conversion.
On Fri, Mar 21, 2008 at 02:50:01PM +0100, Thomas Gleixner wrote:
> On Thu, 20 Mar 2008, Roland McGrath wrote:
> > > Wouldn't it be nicer to just let "arch_ptrace()" return a flag saying
> > > whether it handled things or not?
> >
> > It would certainly be nicer. I would prefer:
> >
> > extern int arch_ptrace(struct task_struct *child, long request,
> > long addr, long data, long *retval);
> >
> > where it returns an error code or it returns 0 and *retval is the value
> > or it returns 1 and it didn't do anything.
> >
> > So this ugliness seemed like a better bet than waiting for 20 more
> > arch sign-offs before any of it could go in. You are certainly in a
> > position to just change the generic signature and make every arch do
> > the update (or fix your typos if you just tweak them all blind), and
> > let them grumble. I did not presume to do so.
>
> What about adding a CONFIG_ARCH_HAS_PTRACE2, which is set by the archs
> which are converted. For those which are not you add a fallback
> implementation:
HAVE_PTRACE2 or at least following the HAVE_* semnatic.
And then do:
config HAVE_PTRACE2
def_bool n
In some common file.
Then arch files can do:
config X86
...
+ select HAVE_PTRACE2
Sam
On Thu, 20 Mar 2008, Roland McGrath wrote:
>
> The motivation is to get the arch function out of the code path for the
> machine-independent request handling. I want to be able to change the
> implementation later without touching the arch code again.
If there is no stronger motivation than that, then I don't want to have
this ugly and unnecessary complication.
I really don't see the advantage of doing
/* We can't handle it, let the generic code sort it out */
return -ENOSYS;
over a
/* We can't handle it, let the generic code sort it out */
return ptrace_request(child, request, addr, data);
in the arch-specific code, and I think the latter version is *much*
preferable if it avoids this whole unnecessary new abstraction crud and
odd testing in the generic part.
> The reason I took the approach I did instead is incrementalism.
> I can't change that signature without breaking about 22 arch builds.
Don't worry about the arch builds. If that's your main worry and reason
for this, it's not worth it. Yes, ptrace changes, and yes, we may have
arch issues, but no, it's not that big of a deal. Just break them.
Make sure x86[-64] works, and make sure that other architectures *can*
work (and explain it on linux-arch) when you have to fix something up, but
ptrace is a blip on the radar for people, it's not going to be a huge
issue.
> I'm only really prepared to thoroughly verify a change on 2 of those
> myself.
.. and you really aren't expected to. It's why we have architecture
people: if there is a good reason for the change (ie it's not just some
churn for churns sake), it's largely up to them to make sure the code
works.
Sure, you need to be able to explain the interface changes and answer
questions, but as mentioned, the ptrace code is not a big deal: it has
lots of tiny _details_, but it's not conceptually complex code.
> So this ugliness seemed like a better bet than waiting for 20 more
> arch sign-offs before any of it could go in.
Really, that has never been a major concern. I will _happily_ break the
odd architectures if I see that it's a matter of changing over some
detail (ie it's not some fundamentally hard issue for an arch maintaner to
fix up).
I think it's *wrong* to add a new an odd calling convention that is less
readable than what we have now, just to try to avoid something like this
that really isn't a big problem. It's not like we haven't broken
architectures over ptrace in the past, and it's also not like it's an area
that I think anybody has ever lost sleep over..
Linus
* Linus Torvalds <[email protected]> wrote:
> > The reason I took the approach I did instead is incrementalism. I
> > can't change that signature without breaking about 22 arch builds.
>
> Don't worry about the arch builds. If that's your main worry and
> reason for this, it's not worth it. Yes, ptrace changes, and yes, we
> may have arch issues, but no, it's not that big of a deal. Just break
> them.
>
> Make sure x86[-64] works, and make sure that other architectures *can*
> work (and explain it on linux-arch) when you have to fix something up,
> but ptrace is a blip on the radar for people, it's not going to be a
> huge issue.
for a long time all the nice but intrusive utrace improvements from
Roland had this big adoption barrier. So Roland went around that and
with a lot of effort he made it optional, incremental and per arch, so
that we could try it on x86 and merge it upstream.
Now that we see how cleaner it is and that it actually was an almost
regression-free endevour on x86 (we had 2 regressions so far, both fixed
- which is an amazing feat for such wide changes IMO), i very much agree
that we should just do the "rest" of this in one big step.
it's a bit of a chicken & egg problem for such changes. If it breaks
architectures it gets dropped out of -mm - even though 90% of our
developers, 95% of our testers and 99% of our active users are using x86
only.
but in general, prototyping something on a single architecture in the
first step is OK - and maybe even the first merge is OK. But once it has
been proven on the most tested Linux platform that we have (and there's
no blatant x86-ism in it), there's no reason not to mandate it.
Ingo
Roland McGrath writes:
> Define the forced_successful_syscall_return() macro to
> pair with our force_successful_syscall_return() definition.
>
> Signed-off-by: Roland McGrath <[email protected]>
Looks OK to me, but what's it useful for?
Paul.