[PATCH 1/8] ptrace: forced_successful_syscall_return() macro

[PATCH 1/8] ptrace: forced_successful_syscall_return() macro

[Roland McGrath]

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 <roland@redhat.com>
---
 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
 
 /*

[PATCH 2/8] alpha ptrace: forced_successful_syscall_return()

[Roland McGrath]

Define the forced_successful_syscall_return() macro to
pair with our force_successful_syscall_return() definition.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 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
 

[PATCH 3/8] ia64 ptrace: forced_successful_syscall_return()

[Roland McGrath]

Define the forced_successful_syscall_return() macro to
pair with our force_successful_syscall_return() definition.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 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 */

[PATCH 4/8] powerpc ptrace: forced_successful_syscall_return()

[Roland McGrath]

Define the forced_successful_syscall_return() macro to
pair with our force_successful_syscall_return() definition.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 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);

Re: [PATCH 4/8] powerpc ptrace: forced_successful_syscall_return()

[Paul Mackerras]

Roland McGrath writes:

> Define the forced_successful_syscall_return() macro to
> pair with our force_successful_syscall_return() definition.
> 
> Signed-off-by: Roland McGrath <roland@redhat.com>

Looks OK to me, but what's it useful for?

Paul.

[PATCH 5/8] sparc64 ptrace: forced_successful_syscall_return()

[Roland McGrath]

Define the forced_successful_syscall_return() macro to
pair with our force_successful_syscall_return() definition.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 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])

Re: [PATCH 5/8] sparc64 ptrace: forced_successful_syscall_return()

[David Miller]

From: Roland McGrath <roland@redhat.com>
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 <roland@redhat.com>

Acked-by: David S. Miller <davem@davemloft.net>

[PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Roland McGrath]

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 <roland@redhat.com>
---
 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();

Re: [PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Linus Torvalds]

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

Re: [PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Christoph Hellwig]

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(...);

Re: [PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Roland McGrath]

> 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

Re: [PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Thomas Gleixner]

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

Re: [PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Christoph Hellwig]

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.

Re: [PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Sam Ravnborg]

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

Re: [PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Linus Torvalds]

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

Re: [PATCH 6/8] ptrace: arch_ptrace -ENOSYS return

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> 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

[PATCH 7/8] x86 ptrace: arch_ptrace -ENOSYS return

[Roland McGrath]

Return -ENOSYS from arch_ptrace and compat_arch_ptrace
rather than calling ptrace_request or compat_ptrace_request.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 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;

[PATCH 8/8] powerpc ptrace: arch_ptrace -ENOSYS return

[Roland McGrath]

Return -ENOSYS from arch_ptrace and compat_arch_ptrace
rather than calling ptrace_request or compat_ptrace_request.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 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;
 	}