arch/ && tracehook_report_syscall_xxx()

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.

@@ -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);
 }

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 disagree. Let the arch code get with the modern style. It is just a minute's hack for the arch maintainer.

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

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.

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

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

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

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.

Christoph has already poked everyone, I believe.

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

Thanks,
Roland

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

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?

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

Ah, good.

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

Oleg.

Leave that one to me.

David