arch/ && tracehook_report_syscall_xxx()

arch/ && tracehook_report_syscall_xxx()

[Oleg Nesterov]

We have a lot of code like arch/alpha/kernel/ptrace.c:syscall_trace()
in arch/ and I can't see how to convert them to use tracehooks.

The first problem, we don't know which hook should be called, there is
no entry/exit argument.

Still, I think it is better to change this code right now, and call
ptrace_report_syscall() directly.

But, the second problem, there is no "struct pt_regs *". What do you
think about the patch below?

However. I can't imagine how ptrace_report_syscall(regs) can actually
use "regs". Perhaps we can remove this argument?

Or what else do you think?

Hmm... and I can't understand how to change
arch/mn10300/kernel/ptrace.c:do_syscall_trace(), it does something
strange with TIF_SINGLESTEP. (maintainers cc'ed).

Oleg.

--- PTRACE/arch/alpha/kernel/ptrace.c~1_alpha	2009-04-06 00:03:35.000000000 +0200
+++ PTRACE/arch/alpha/kernel/ptrace.c	2009-04-27 19:11:50.000000000 +0200
@@ -11,6 +11,7 @@
 #include <linux/smp_lock.h>
 #include <linux/errno.h>
 #include <linux/ptrace.h>
+#include <linux/tracehook.h>
 #include <linux/user.h>
 #include <linux/slab.h>
 #include <linux/security.h>
@@ -354,20 +355,6 @@ syscall_trace(void)
 {
 	if (!test_thread_flag(TIF_SYSCALL_TRACE))
 		return;
-	if (!(current->ptrace & PT_PTRACED))
-		return;
-	/* The 0x80 provides a way for the tracing parent to distinguish
-	   between a syscall stop and SIGTRAP delivery */
-	ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
-				 ? 0x80 : 0));
 
-	/*
-	 * This isn't the same as continuing with a signal, but it will do
-	 * for normal use.  strace only continues with a signal if the
-	 * stopping signal is not SIGTRAP.  -brl
-	 */
-	if (current->exit_code) {
-		send_sig(current->exit_code, current, 1);
-		current->exit_code = 0;
-	}
+	ptrace_report_syscall(NULL);
 }

Re: arch/ && tracehook_report_syscall_xxx()

[Roland McGrath]

> We have a lot of code like arch/alpha/kernel/ptrace.c:syscall_trace()
> in arch/ and I can't see how to convert them to use tracehooks.
> 
> The first problem, we don't know which hook should be called, there is
> no entry/exit argument.

These arch maintainers just need to update their code.  Christoph has
started poking arch folks individually about getting up to speed.

IMHO, it is better anyway to use separate entry/exit calls.  For that
change, it is often easy to see how to do it correctly in the assembly code
without really knowing the arch at all.  (There are separate assembly paths
leading to the calls for entry vs exit cases already, just change the
symbol names.  Adding an argument would require a bit of a clue about
assembly on the arch.)

> Still, I think it is better to change this code right now, and call
> ptrace_report_syscall() directly.

I disagree.  Let the arch code get with the modern style.  
It is just a minute's hack for the arch maintainer.

> But, the second problem, there is no "struct pt_regs *".

They can use task_pt_regs(), it gets the same pointer.  It's passed in as
an argument because usually the arch really does have it handy right there
in a register so it's cheaper than recalculating.  (All the non-ancient
arch code uses it there for audit_syscall_{entry,exit} calls too.)

> However. I can't imagine how ptrace_report_syscall(regs) can actually
> use "regs". Perhaps we can remove this argument?

ptrace_report_syscall doesn't need it, it could be removed.  But that is
not apropos to the arch code, which should use tracehook_* properly.

> Hmm... and I can't understand how to change
> arch/mn10300/kernel/ptrace.c:do_syscall_trace(), it does something
> strange with TIF_SINGLESTEP. (maintainers cc'ed).

We can leave those details to the arch maintainers.  That case looks to me
like old hacks for step-over-syscall behavior, which made the opposite
choice (between |0x80 or not for stepping) from what x86 made.  Probably
they just want to match the new "norms" and not try to do anything
different for single-step in syscall tracing.


Thanks,
Roland

Re: arch/ && tracehook_report_syscall_xxx()

[Christoph Hellwig]

On Mon, Apr 27, 2009 at 11:29:19AM -0700, Roland McGrath wrote:
> > We have a lot of code like arch/alpha/kernel/ptrace.c:syscall_trace()
> > in arch/ and I can't see how to convert them to use tracehooks.
> > 
> > The first problem, we don't know which hook should be called, there is
> > no entry/exit argument.
> 
> These arch maintainers just need to update their code.  Christoph has
> started poking arch folks individually about getting up to speed.
> 
> IMHO, it is better anyway to use separate entry/exit calls.  For that
> change, it is often easy to see how to do it correctly in the assembly code
> without really knowing the arch at all.  (There are separate assembly paths
> leading to the calls for entry vs exit cases already, just change the
> symbol names.  Adding an argument would require a bit of a clue about
> assembly on the arch.)

I've poked a few arch maintainers in the past to separate the enter/exit
path and usually got the desired changes :)

> > Still, I think it is better to change this code right now, and call
> > ptrace_report_syscall() directly.
> 
> I disagree.  Let the arch code get with the modern style.  
> It is just a minute's hack for the arch maintainer.

Yeah, let's get the architectures up to modern standards first, that
should make life a lot simpler.

Fwd: arch/ && tracehook_report_syscall_xxx()

[Oleg Nesterov]

On 04/27, Christoph Hellwig wrote:
>
> I've poked a few arch maintainers in the past to separate the enter/exit
> path and usually got the desired changes :)

I'd like to try your method!

So. Dear maintainers of

	alpha
	arm
	avr32
	blackfin
	cris
	h8300
	m32r
	m68k
	m68knommu
	mips
	parisc
	um
	xtensa

. Could you please convert your syscall trace code to use
tracehook_report_syscall_entry/tracehook_report_syscall_exit ?


For example, let's look at more or less typical arch/alpha/kernel/ptrace.c,

	asmlinkage void
	syscall_trace(void)
	{
		if (!test_thread_flag(TIF_SYSCALL_TRACE))
			return;
		if (!(current->ptrace & PT_PTRACED))
			return;
		/* The 0x80 provides a way for the tracing parent to distinguish
		   between a syscall stop and SIGTRAP delivery */
		ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
					 ? 0x80 : 0));

		/*
		 * This isn't the same as continuing with a signal, but it will do
		 * for normal use.  strace only continues with a signal if the
		 * stopping signal is not SIGTRAP.  -brl
		 */
		if (current->exit_code) {
			send_sig(current->exit_code, current, 1);
			current->exit_code = 0;
		}
	}

it would be really nice to turn it into something like

	asmlinkage void
	syscall_trace(int entryexit)
	{
		if (!test_thread_flag(TIF_SYSCALL_TRACE))
			return;

		if (entryexit)
			tracehook_report_syscall_entry(task_pt_regs(current));
		else
			tracehook_report_syscall_exit(task_pt_regs(current), stepping);
	}

Also, tracehook_report_syscall_entry() might want to abort this system
call (please see the comment above this helper), it would be great to
take the returned value into account.


arch/* play with ptrace internals which should be changed soon, not good.

Thanks!

Oleg.

Re: Fwd: arch/ && tracehook_report_syscall_xxx()

[Roland McGrath]

Christoph has already poked everyone, I believe.

> it would be really nice to turn it into something like
> 
> 	asmlinkage void
> 	syscall_trace(int entryexit)

We recommend replacing this with separate entry/exit paths from the
assembly code, if you are changing arch code anyway.


Thanks,
Roland

Re: Fwd: arch/ && tracehook_report_syscall_xxx()

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 04/27, Christoph Hellwig wrote:
> >
> > I've poked a few arch maintainers in the past to separate the enter/exit
> > path and usually got the desired changes :)
> 
> I'd like to try your method!
> 
> So. Dear maintainers of
> 
> 	alpha
> 	arm
> 	avr32
> 	blackfin
> 	cris
> 	h8300
> 	m32r
> 	m68k
> 	m68knommu
> 	mips
> 	parisc
> 	um
> 	xtensa
> 
> . Could you please convert your syscall trace code to use
> tracehook_report_syscall_entry/tracehook_report_syscall_exit ?
> 
> 
> For example, let's look at more or less typical arch/alpha/kernel/ptrace.c,
> 
> 	asmlinkage void
> 	syscall_trace(void)
> 	{
> 		if (!test_thread_flag(TIF_SYSCALL_TRACE))
> 			return;
> 		if (!(current->ptrace & PT_PTRACED))
> 			return;
> 		/* The 0x80 provides a way for the tracing parent to distinguish
> 		   between a syscall stop and SIGTRAP delivery */
> 		ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
> 					 ? 0x80 : 0));
> 
> 		/*
> 		 * This isn't the same as continuing with a signal, but it will do
> 		 * for normal use.  strace only continues with a signal if the
> 		 * stopping signal is not SIGTRAP.  -brl
> 		 */
> 		if (current->exit_code) {
> 			send_sig(current->exit_code, current, 1);
> 			current->exit_code = 0;
> 		}
> 	}
> 
> it would be really nice to turn it into something like
> 
> 	asmlinkage void
> 	syscall_trace(int entryexit)
> 	{
> 		if (!test_thread_flag(TIF_SYSCALL_TRACE))
> 			return;
> 
> 		if (entryexit)
> 			tracehook_report_syscall_entry(task_pt_regs(current));
> 		else
> 			tracehook_report_syscall_exit(task_pt_regs(current), stepping);
> 	}
> 
> Also, tracehook_report_syscall_entry() might want to abort this system
> call (please see the comment above this helper), it would be great to
> take the returned value into account.
> 
> arch/* play with ptrace internals which should be changed soon, not good.

And there's upstream examples in:

 arch/ia64/kernel/ptrace.c
 arch/powerpc/kernel/ptrace.c
 arch/s390/kernel/ptrace.c
 arch/sh/kernel/ptrace_32.c
 arch/sh/kernel/ptrace_64.c
 arch/sparc/kernel/ptrace_32.c
 arch/sparc/kernel/ptrace_64.c
 arch/x86/kernel/ptrace.c

as well, for tracehook usage. Chances are that your architecture's 
syscall entry/exit ptrace hooks look quite similar to one of the 
architectures above!

	Ingo

Re: Fwd: arch/ && tracehook_report_syscall_xxx()

[Christoph Hellwig]

On Wed, Apr 29, 2009 at 09:08:52PM +0200, Oleg Nesterov wrote:
> . Could you please convert your syscall trace code to use
> tracehook_report_syscall_entry/tracehook_report_syscall_exit ?

This is part of Roland's step by step cookbook for new-style ptrace,
so we should have most done.  Is there anything that makes this more
urgent than the others steps for you?

Re: Fwd: arch/ && tracehook_report_syscall_xxx()

[Kyle McMartin]

On Wed, Apr 29, 2009 at 09:08:52PM +0200, Oleg Nesterov wrote:
> So. Dear maintainers of
> 
> 	parisc
> 
> . Could you please convert your syscall trace code to use
> tracehook_report_syscall_entry/tracehook_report_syscall_exit ?
> 

I've got this mostly completed and working on parisc (at least, gdb
and strace continue working, so I'm happy with that result... :)

I'll probably push it for .30 since it seems harmless.

--Kyle

Re: arch/ && tracehook_report_syscall_xxx()

[Oleg Nesterov]

On 04/27, Roland McGrath wrote:
>
> > We have a lot of code like arch/alpha/kernel/ptrace.c:syscall_trace()
> > in arch/ and I can't see how to convert them to use tracehooks.
> >
> > The first problem, we don't know which hook should be called, there is
> > no entry/exit argument.
>
> These arch maintainers just need to update their code.  Christoph has
> started poking arch folks individually about getting up to speed.

Ah, good.

> IMHO, it is better anyway to use separate entry/exit calls.  For that
> change, it is often easy to see how to do it correctly in the assembly code
> without really knowing the arch at all.  (There are separate assembly paths
> leading to the calls for entry vs exit cases already, just change the
> symbol names.  Adding an argument would require a bit of a clue about
> assembly on the arch.)
>
> > Still, I think it is better to change this code right now, and call
> > ptrace_report_syscall() directly.
>
> I disagree.  Let the arch code get with the modern style.
> It is just a minute's hack for the arch maintainer.

Sure, if maintainers can help then we don't need the temporary/incomplete
changes.

Oleg.

Re: arch/ && tracehook_report_syscall_xxx()

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> Hmm... and I can't understand how to change
> arch/mn10300/kernel/ptrace.c:do_syscall_trace(), it does something
> strange with TIF_SINGLESTEP. (maintainers cc'ed).

Leave that one to me.

David