[RFC] ARM support for syscall tracing

[RFC] ARM support for syscall tracing

[Steven Walter]

I recently noticed that "perf test" fails tests #2 - #4 on ARM.  In
investigating that failure, I noticed that those tests rely on
FTRACE_SYSCALLS, which was not available on ARM.

It didn't look like too much work to enable HAVE_SYSCALL_TRACEPOINTS, so
that's what I did in the patches that follow.  This is a
request-for-comment because I don't entirely understand some of the
changes I made, so it's quite likely that some parts are either
out-and-out wrong, or at least can be done a better way.

That said, the tests in "perf test" pass except for #1 (kallsyms), and
strace still seems to report sensible values for system calls.

Comments?

[PATCH 1/3] ARM: add support for the generic syscall.h interface

[Steven Walter]

Supplying the asm-generic/syscall.h interface is a pre-requisite for
HAVE_ARCH_TRACEHOOK

Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
---
 arch/arm/include/asm/syscall.h |   78 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 78 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/syscall.h

diff --git a/arch/arm/include/asm/syscall.h b/arch/arm/include/asm/syscall.h
new file mode 100644
index 0000000..f104339
--- /dev/null
+++ b/arch/arm/include/asm/syscall.h
@@ -0,0 +1,78 @@
+/*
+ * Access to user system call parameters and results
+ *
+ * See asm-generic/syscall.h for descriptions of what we must do here.
+ */
+
+#ifndef _ASM_ARM_SYSCALL_H
+#define _ASM_ARM_SYSCALL_H
+
+#include <linux/err.h>
+
+extern const unsigned long sys_call_table[];
+
+static inline int syscall_get_nr(struct task_struct *task,
+				 struct pt_regs *regs)
+{
+	return task_thread_info(task)->syscall;
+}
+
+static inline void syscall_rollback(struct task_struct *task,
+				    struct pt_regs *regs)
+{
+	regs->ARM_r0 = regs->ARM_ORIG_r0;
+}
+
+static inline long syscall_get_error(struct task_struct *task,
+				     struct pt_regs *regs)
+{
+	unsigned long error = regs->ARM_r0;
+	return IS_ERR_VALUE(error) ? error : 0;
+}
+
+static inline long syscall_get_return_value(struct task_struct *task,
+					    struct pt_regs *regs)
+{
+	return regs->ARM_r0;
+}
+
+static inline void syscall_set_return_value(struct task_struct *task,
+					    struct pt_regs *regs,
+					    int error, long val)
+{
+	regs->ARM_r0 = (long) error ?: val;
+}
+
+static inline void syscall_get_arguments(struct task_struct *task,
+					 struct pt_regs *regs,
+					 unsigned int i, unsigned int n,
+					 unsigned long *args)
+{
+	BUG_ON(i + n > 6);
+	if (i == 0) {
+		args[0] = regs->ARM_ORIG_r0;
+		args++;
+		i++;
+		n--;
+	}
+
+	memcpy(args, &regs->ARM_r1 + (i - 1), n * sizeof(args[0]));
+}
+
+static inline void syscall_set_arguments(struct task_struct *task,
+					 struct pt_regs *regs,
+					 unsigned int i, unsigned int n,
+					 const unsigned long *args)
+{
+	BUG_ON(i + n > 6);
+	if (i == 0) {
+		regs->ARM_ORIG_r0 = args[0];
+		args++;
+		i++;
+		n--;
+	}
+
+	memcpy(&regs->ARM_r1 + (i - 1), args, n * sizeof(args[0]));
+}
+
+#endif	/* _ASM_ARM_SYSCALL_H */
-- 
1.7.5.4.2.g81f75

[PATCH 2/3] ARM: add TRACEHOOK support

[Steven Walter]

Add calls to tracehook_report_syscall_{entry,exit} and tracehook_signal_handler

Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
---
 arch/arm/Kconfig              |    1 +
 arch/arm/include/asm/ptrace.h |    5 +++++
 arch/arm/kernel/ptrace.c      |   21 +++++++--------------
 arch/arm/kernel/signal.c      |    2 ++
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index b65b910..0a93d76 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -10,6 +10,7 @@ config ARM
 	select GENERIC_ATOMIC64 if (CPU_V6 || !CPU_32v6K || !AEABI)
 	select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
 	select HAVE_ARCH_KGDB
+	select HAVE_ARCH_TRACEHOOK
 	select HAVE_KPROBES if (!XIP_KERNEL && !THUMB2_KERNEL)
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
 	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index 312d108..98ea76d 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h
@@ -235,6 +235,11 @@ static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
 	return regs->ARM_sp;
 }
 
+static inline unsigned long user_stack_pointer(struct pt_regs *regs)
+{
+	return regs->ARM_sp;
+}
+
 #endif /* __KERNEL__ */
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 9726006..1411848 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -22,6 +22,7 @@
 #include <linux/perf_event.h>
 #include <linux/hw_breakpoint.h>
 #include <linux/regset.h>
+#include <linux/tracehook.h>
 
 #include <asm/pgtable.h>
 #include <asm/system.h>
@@ -928,8 +929,6 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 
 	if (!test_thread_flag(TIF_SYSCALL_TRACE))
 		return scno;
-	if (!(current->ptrace & PT_PTRACED))
-		return scno;
 
 	/*
 	 * Save IP.  IP is used to denote syscall entry/exit:
@@ -940,19 +939,13 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 
 	current_thread_info()->syscall = scno;
 
-	/* 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;
+	if (why == 0) {
+		if (tracehook_report_syscall_entry(regs))
+			current_thread_info()->syscall = -1;
+	} else {
+		tracehook_report_syscall_exit(regs, 0);
 	}
+
 	regs->ARM_ip = ip;
 
 	return current_thread_info()->syscall;
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 0340224..26c26f5 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -645,6 +645,8 @@ handle_signal(unsigned long sig, struct k_sigaction *ka,
 	recalc_sigpending();
 	spin_unlock_irq(&tsk->sighand->siglock);
 
+	tracehook_signal_handler(sig, info, ka, regs, 0);
+
 	return 0;
 }
 
-- 
1.7.5.4.2.g81f75

[PATCH 2/3] ARM: add TRACEHOOK support

[Will Deacon]

Hi Steven,

On Tue, Nov 29, 2011 at 04:28:14PM +0000, Steven Walter wrote:
> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index 9726006..1411848 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -22,6 +22,7 @@
>  #include <linux/perf_event.h>
>  #include <linux/hw_breakpoint.h>
>  #include <linux/regset.h>
> +#include <linux/tracehook.h>
>  
>  #include <asm/pgtable.h>
>  #include <asm/system.h>
> @@ -928,8 +929,6 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
>  
>  	if (!test_thread_flag(TIF_SYSCALL_TRACE))
>  		return scno;
> -	if (!(current->ptrace & PT_PTRACED))
> -		return scno;

This means that we can potentially corrupt current_thread_info()->syscall
for tasks that aren't being traced. I don't think that matters as it's only
used by ptrace, but worth bearing in mind.

>  	/*
>  	 * Save IP.  IP is used to denote syscall entry/exit:
> @@ -940,19 +939,13 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
>  
>  	current_thread_info()->syscall = scno;
>  
> -	/* 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;
> +	if (why == 0) {
> +		if (tracehook_report_syscall_entry(regs))
> +			current_thread_info()->syscall = -1;

Why do you set syscall to -1 here? It looks like
tracehook_report_syscall_entry always returns 0, but even so, I'm not sure
what this -1 represents. You could also rewrite this a bit more neatly:

if (why)
	tracehook_report_syscall_exit(...);
else
	tracehook_report_syscall_entry(...);


> +	} else {
> +		tracehook_report_syscall_exit(regs, 0);
>  	}
> +
>  	regs->ARM_ip = ip;
>  
>  	return current_thread_info()->syscall;
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 0340224..26c26f5 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -645,6 +645,8 @@ handle_signal(unsigned long sig, struct k_sigaction *ka,
>  	recalc_sigpending();
>  	spin_unlock_irq(&tsk->sighand->siglock);
>  
> +	tracehook_signal_handler(sig, info, ka, regs, 0);
> +

This doesn't appear to do anything but I guess that's where it should live.

Will

[PATCH 2/3] ARM: add TRACEHOOK support

[Steven Walter]

On Tue, Nov 29, 2011 at 12:04 PM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Steven,
>
> On Tue, Nov 29, 2011 at 04:28:14PM +0000, Steven Walter wrote:
>> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
>> index 9726006..1411848 100644
>> --- a/arch/arm/kernel/ptrace.c
>> +++ b/arch/arm/kernel/ptrace.c
>> @@ -22,6 +22,7 @@
>> ?#include <linux/perf_event.h>
>> ?#include <linux/hw_breakpoint.h>
>> ?#include <linux/regset.h>
>> +#include <linux/tracehook.h>
>>
>> ?#include <asm/pgtable.h>
>> ?#include <asm/system.h>
>> @@ -928,8 +929,6 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
>>
>> ? ? ? if (!test_thread_flag(TIF_SYSCALL_TRACE))
>> ? ? ? ? ? ? ? return scno;
>> - ? ? if (!(current->ptrace & PT_PTRACED))
>> - ? ? ? ? ? ? return scno;
>
> This means that we can potentially corrupt current_thread_info()->syscall
> for tasks that aren't being traced. I don't think that matters as it's only
> used by ptrace, but worth bearing in mind.

Is it valid for a process to have TIF_SYSCALL_TRACE set but not
PT_PTRACED?  Given that they were checking both before, one might
assume so, but I don't really know what that would mean.

>> ? ? ? /*
>> ? ? ? ?* Save IP. ?IP is used to denote syscall entry/exit:
>> @@ -940,19 +939,13 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
>>
>> ? ? ? current_thread_info()->syscall = scno;
>>
>> - ? ? /* 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;
>> + ? ? if (why == 0) {
>> + ? ? ? ? ? ? if (tracehook_report_syscall_entry(regs))
>> + ? ? ? ? ? ? ? ? ? ? current_thread_info()->syscall = -1;
>
> Why do you set syscall to -1 here? It looks like
> tracehook_report_syscall_entry always returns 0, but even so, I'm not sure
> what this -1 represents. You could also rewrite this a bit more neatly:

tracehook_report_syscall_entry is marked __must_check, and the comment
says that if it returns non-zero the arch could should abort the
system call.  x86 just returns -1 from its trace function in that
case, so I took the same approach.  In particular, if we treat -1 as
the system call number, the system call should fail with ENOSYS, which
include/linux/tracehook.h says is acceptable.

> if (why)
> ? ? ? ?tracehook_report_syscall_exit(...);
> else
> ? ? ? ?tracehook_report_syscall_entry(...);

Agreed, will fix

>> + ? ? } else {
>> + ? ? ? ? ? ? tracehook_report_syscall_exit(regs, 0);
>> ? ? ? }
>> +
>> ? ? ? regs->ARM_ip = ip;
>>
>> ? ? ? return current_thread_info()->syscall;
>> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
>> index 0340224..26c26f5 100644
>> --- a/arch/arm/kernel/signal.c
>> +++ b/arch/arm/kernel/signal.c
>> @@ -645,6 +645,8 @@ handle_signal(unsigned long sig, struct k_sigaction *ka,
>> ? ? ? recalc_sigpending();
>> ? ? ? spin_unlock_irq(&tsk->sighand->siglock);
>>
>> + ? ? tracehook_signal_handler(sig, info, ka, regs, 0);
>> +
>
> This doesn't appear to do anything but I guess that's where it should live.

Right, I just added it because arch/Kconfig said to :-)
-- 
-Steven Walter <stevenrwalter@gmail.com>

[PATCH 3/3] ARM: support syscall tracing

[Steven Walter]

As specified by ftrace-design.txt, TIF_SYSCALL_TRACEPOINT was added, as
well as NR_syscalls in asm/unistd.h.  Additionally, __sys_trace was
modified to call trace_sys_enter and trace_sys_exit when appropriate.

There was previously an assembler variable named NR_syscalls in
entry-common.S.  This was renamed NR_syscalls_asm so as not to collide
with NR_syscalls from asm/unistd.h.

Tests #2 - #4 of "perf test" now complete successfully.

Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
---
 arch/arm/Kconfig                   |    1 +
 arch/arm/include/asm/thread_info.h |    2 ++
 arch/arm/include/asm/unistd.h      |    2 ++
 arch/arm/kernel/entry-common.S     |   14 ++++++++------
 arch/arm/kernel/ptrace.c           |   24 ++++++++++++++++++------
 5 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 0a93d76..7f0c53b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -11,6 +11,7 @@ config ARM
 	select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_TRACEHOOK
+	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_KPROBES if (!XIP_KERNEL && !THUMB2_KERNEL)
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
 	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 7b5cc8d..17a84b6 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -145,6 +145,7 @@ extern void vfp_flush_hwstate(struct thread_info *);
 #define TIF_FREEZE		19
 #define TIF_RESTORE_SIGMASK	20
 #define TIF_SECCOMP		21
+#define TIF_SYSCALL_TRACEPOINT	22
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
@@ -155,6 +156,7 @@ extern void vfp_flush_hwstate(struct thread_info *);
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
 #define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
 
 /*
  * Change these and you break ASM code in entry-common.S
diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index 2c04ed5..1f23923 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -403,6 +403,8 @@
 #define __NR_sendmmsg			(__NR_SYSCALL_BASE+374)
 #define __NR_setns			(__NR_SYSCALL_BASE+375)
 
+#define NR_syscalls (__NR_setns+1)
+
 /*
  * The following SWIs are ARM private.
  */
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index b2a27b6..314280d 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -88,6 +88,7 @@ ENTRY(ret_from_fork)
 	ldr	r1, [tsk, #TI_FLAGS]		@ check for syscall tracing
 	mov	why, #1
 	tst	r1, #_TIF_SYSCALL_TRACE		@ are we tracing syscalls?
+	tsteq	r1, #_TIF_SYSCALL_TRACEPOINT
 	beq	ret_slow_syscall
 	mov	r1, sp
 	mov	r0, #1				@ trace exit [IP = 1]
@@ -95,8 +96,8 @@ ENTRY(ret_from_fork)
 	b	ret_slow_syscall
 ENDPROC(ret_from_fork)
 
-	.equ NR_syscalls,0
-#define CALL(x) .equ NR_syscalls,NR_syscalls+1
+	.equ NR_syscalls_asm,0
+#define CALL(x) .equ NR_syscalls_asm,NR_syscalls_asm+1
 #include "calls.S"
 #undef CALL
 #define CALL(x) .long x
@@ -443,10 +444,11 @@ ENTRY(vector_swi)
 1:
 #endif
 
-	tst	r10, #_TIF_SYSCALL_TRACE		@ are we tracing syscalls?
+	tst	r10, #_TIF_SYSCALL_TRACE	@ are we tracing syscalls?
+	tsteq	r10, #_TIF_SYSCALL_TRACEPOINT
 	bne	__sys_trace
 
-	cmp	scno, #NR_syscalls		@ check upper syscall limit
+	cmp	scno, #NR_syscalls_asm		@ check upper syscall limit
 	adr	lr, BSYM(ret_fast_syscall)	@ return address
 	ldrcc	pc, [tbl, scno, lsl #2]		@ call sys_* routine
 
@@ -471,7 +473,7 @@ __sys_trace:
 	adr	lr, BSYM(__sys_trace_return)	@ return address
 	mov	scno, r0			@ syscall number (possibly new)
 	add	r1, sp, #S_R0 + S_OFF		@ pointer to regs
-	cmp	scno, #NR_syscalls		@ check upper syscall limit
+	cmp	scno, #NR_syscalls_asm		@ check upper syscall limit
 	ldmccia	r1, {r0 - r3}			@ have to reload r0 - r3
 	ldrcc	pc, [tbl, scno, lsl #2]		@ call sys_* routine
 	b	2b
@@ -517,7 +519,7 @@ ENTRY(sys_call_table)
 sys_syscall:
 		bic	scno, r0, #__NR_OABI_SYSCALL_BASE
 		cmp	scno, #__NR_syscall - __NR_SYSCALL_BASE
-		cmpne	scno, #NR_syscalls	@ check range
+		cmpne	scno, #NR_syscalls_asm	@ check range
 		stmloia	sp, {r5, r6}		@ shuffle args
 		movlo	r0, r1
 		movlo	r1, r2
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 1411848..0e2699a 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -28,6 +28,9 @@
 #include <asm/system.h>
 #include <asm/traps.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/syscalls.h>
+
 #define REG_PC	15
 #define REG_PSR	16
 /*
@@ -927,7 +930,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 {
 	unsigned long ip;
 
-	if (!test_thread_flag(TIF_SYSCALL_TRACE))
+	if (!test_thread_flag(TIF_SYSCALL_TRACE) && !test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		return scno;
 
 	/*
@@ -939,11 +942,20 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 
 	current_thread_info()->syscall = scno;
 
-	if (why == 0) {
-		if (tracehook_report_syscall_entry(regs))
-			current_thread_info()->syscall = -1;
-	} else {
-		tracehook_report_syscall_exit(regs, 0);
+	if (test_thread_flag(TIF_SYSCALL_TRACE)) {
+		if (why == 0) {
+			if (tracehook_report_syscall_entry(regs))
+				current_thread_info()->syscall = -1;
+		} else {
+			tracehook_report_syscall_exit(regs, 0);
+		}
+	}
+
+	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT)) {
+		if (why == 0)
+			trace_sys_enter(regs, scno);
+		else
+			trace_sys_exit(regs, scno);
 	}
 
 	regs->ARM_ip = ip;
-- 
1.7.5.4.2.g81f75

[PATCH 3/3] ARM: support syscall tracing

[Will Deacon]

On Tue, Nov 29, 2011 at 04:28:15PM +0000, Steven Walter wrote:
> diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
> index 2c04ed5..1f23923 100644
> --- a/arch/arm/include/asm/unistd.h
> +++ b/arch/arm/include/asm/unistd.h
> @@ -403,6 +403,8 @@
>  #define __NR_sendmmsg			(__NR_SYSCALL_BASE+374)
>  #define __NR_setns			(__NR_SYSCALL_BASE+375)
>  
> +#define NR_syscalls (__NR_setns+1)

[...]

> -	.equ NR_syscalls,0
> -#define CALL(x) .equ NR_syscalls,NR_syscalls+1
> +	.equ NR_syscalls_asm,0
> +#define CALL(x) .equ NR_syscalls_asm,NR_syscalls_asm+1

If we need to have two definitions of NR_syscalls, then it's probably best
to define one in terms of the other to ensure they are consistent.
Unfortunately, it looks like we calculate NR_syscalls recursively from the
syscall table. Perhaps you'd be better off with just the #define and have a
sanity check on the table size using that.

> -	tst	r10, #_TIF_SYSCALL_TRACE		@ are we tracing syscalls?
> +	tst	r10, #_TIF_SYSCALL_TRACE	@ are we tracing syscalls?
> +	tsteq	r10, #_TIF_SYSCALL_TRACEPOINT
>  	bne	__sys_trace

Maybe you could macroise this pattern.

> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index 1411848..0e2699a 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -28,6 +28,9 @@
>  #include <asm/system.h>
>  #include <asm/traps.h>
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/syscalls.h>

[...]

> @@ -939,11 +942,20 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
>  
>  	current_thread_info()->syscall = scno;
>  
> -	if (why == 0) {
> -		if (tracehook_report_syscall_entry(regs))
> -			current_thread_info()->syscall = -1;
> -	} else {
> -		tracehook_report_syscall_exit(regs, 0);

Wait - didn't you just add this code?

Will

[PATCH 3/3] ARM: support syscall tracing

[Steven Walter]

(Oops, dropped l-a-k from CC: )

On Tue, Nov 29, 2011 at 12:24 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Nov 29, 2011 at 04:28:15PM +0000, Steven Walter wrote:
>> diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
>> index 2c04ed5..1f23923 100644
>> --- a/arch/arm/include/asm/unistd.h
>> +++ b/arch/arm/include/asm/unistd.h
>> @@ -403,6 +403,8 @@
>>  #define __NR_sendmmsg                        (__NR_SYSCALL_BASE+374)
>>  #define __NR_setns                   (__NR_SYSCALL_BASE+375)
>>
>> +#define NR_syscalls (__NR_setns+1)
>
> [...]
>
>> -     .equ NR_syscalls,0
>> -#define CALL(x) .equ NR_syscalls,NR_syscalls+1
>> +     .equ NR_syscalls_asm,0
>> +#define CALL(x) .equ NR_syscalls_asm,NR_syscalls_asm+1
>
> If we need to have two definitions of NR_syscalls, then it's probably best
> to define one in terms of the other to ensure they are consistent.
> Unfortunately, it looks like we calculate NR_syscalls recursively from the
> syscall table. Perhaps you'd be better off with just the #define and have a
> sanity check on the table size using that.

Yeah I'll have to do something about this.  Russell also raised
concern about it.

>> -     tst     r10, #_TIF_SYSCALL_TRACE                @ are we tracing syscalls?
>> +     tst     r10, #_TIF_SYSCALL_TRACE        @ are we tracing syscalls?
>> +     tsteq   r10, #_TIF_SYSCALL_TRACEPOINT
>>       bne     __sys_trace
>
> Maybe you could macroise this pattern.

Good idea.

>> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
>> index 1411848..0e2699a 100644
>> --- a/arch/arm/kernel/ptrace.c
>> +++ b/arch/arm/kernel/ptrace.c
>> @@ -28,6 +28,9 @@
>>  #include <asm/system.h>
>>  #include <asm/traps.h>
>>
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/syscalls.h>
>
> [...]
>
>> @@ -939,11 +942,20 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
>>
>>       current_thread_info()->syscall = scno;
>>
>> -     if (why == 0) {
>> -             if (tracehook_report_syscall_entry(regs))
>> -                     current_thread_info()->syscall = -1;
>> -     } else {
>> -             tracehook_report_syscall_exit(regs, 0);
>
> Wait - didn't you just add this code?

Indeed.  It's not going away, just getting indented.  Previously we
couldn't get to that line unless TIF_SYSCALL_TRACE.  Now, all we know
is TIF_SYSCALL_TRACE || TIF_SYSCALL_TRACEPOINT, so I have to check
which.
-- 
-Steven Walter <stevenrwalter@gmail.com>

[PATCH 3/3] ARM: support syscall tracing

[Russell King - ARM Linux]

On Tue, Nov 29, 2011 at 11:28:15AM -0500, Steven Walter wrote:
> diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
> index 2c04ed5..1f23923 100644
> --- a/arch/arm/include/asm/unistd.h
> +++ b/arch/arm/include/asm/unistd.h
> @@ -403,6 +403,8 @@
>  #define __NR_sendmmsg			(__NR_SYSCALL_BASE+374)
>  #define __NR_setns			(__NR_SYSCALL_BASE+375)
>  
> +#define NR_syscalls (__NR_setns+1)

NAK.  This is a recipe for it being forgotten to be updated, and for it
to become unsynchronized with what is the _real_ number of syscalls,
which is what is in the assembly code.

You're also exporting it to userspace.  It has no business being in
userspace.

Lastly, it's wrong.  __NR_SYSCALL_BASE may be 0 or 0x90000 depending on
the ABI selected.  If it's 0x90000, will tracepoint stuff work or will it
explode because of a stupidly large table somewhere?

kernel/trace/trace_syscalls.c:      for (i = 0; i < NR_syscalls; i++) {

and

kernel/trace/trace_syscalls.c:static DECLARE_BITMAP(enabled_perf_enter_syscalls, NR_syscalls);
kernel/trace/trace_syscalls.c:static DECLARE_BITMAP(enabled_perf_exit_syscalls,NR_syscalls);

all point@this being very wrong.

[PATCH 3/3] ARM: support syscall tracing

[Steven Walter]

Thanks for your comments, responses below

On Tue, Nov 29, 2011 at 12:46 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Nov 29, 2011 at 11:28:15AM -0500, Steven Walter wrote:
>> diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
>> index 2c04ed5..1f23923 100644
>> --- a/arch/arm/include/asm/unistd.h
>> +++ b/arch/arm/include/asm/unistd.h
>> @@ -403,6 +403,8 @@
>> ?#define __NR_sendmmsg ? ? ? ? ? ? ? ? ? ? ? ?(__NR_SYSCALL_BASE+374)
>> ?#define __NR_setns ? ? ? ? ? ? ? ? ? (__NR_SYSCALL_BASE+375)
>>
>> +#define NR_syscalls (__NR_setns+1)
>
> NAK. ?This is a recipe for it being forgotten to be updated, and for it
> to become unsynchronized with what is the _real_ number of syscalls,
> which is what is in the assembly code.

Yes, I like how the assembler counts the number of syscalls for you.
However, every other architecture uses either a bare number or
__NR_last_syscall+1 for defining NR_syscalls.

> You're also exporting it to userspace. ?It has no business being in
> userspace.

That was unintentional, I'll fix.

> Lastly, it's wrong. ?__NR_SYSCALL_BASE may be 0 or 0x90000 depending on
> the ABI selected. ?If it's 0x90000, will tracepoint stuff work or will it
> explode because of a stupidly large table somewhere?

Right you are.  Didn't consider that.  Does that mean that
syscall_get_nr() should return the offset from __NR_SYSCALL_BASE, so
that it will be strictly less than NR_syscalls?

> kernel/trace/trace_syscalls.c: ? ? ?for (i = 0; i < NR_syscalls; i++) {
>
> and
>
> kernel/trace/trace_syscalls.c:static DECLARE_BITMAP(enabled_perf_enter_syscalls, NR_syscalls);
> kernel/trace/trace_syscalls.c:static DECLARE_BITMAP(enabled_perf_exit_syscalls,NR_syscalls);
>
> all point at this being very wrong.

Initially I was going to assign the assembler version of NR_syscalls
to a symbol which could be referenced from C.  However, putting the C
declaration for the variable in unistd.h made the assembler unhappy.
Is it kosher to have C stuff in unistd.h?  Since there was no prior
example of it, I assumed not.  However, perhaps I could just hide it
from the assembler with some kind of #ifdef?

Thanks again
-- 
-Steven Walter <stevenrwalter@gmail.com>

[PATCH 3/3] ARM: support syscall tracing

[Russell King - ARM Linux]

On Tue, Nov 29, 2011 at 01:12:50PM -0500, Steven Walter wrote:
> Yes, I like how the assembler counts the number of syscalls for you.
> However, every other architecture uses either a bare number or
> __NR_last_syscall+1 for defining NR_syscalls.
> 
> > You're also exporting it to userspace. ?It has no business being in
> > userspace.
> 
> That was unintentional, I'll fix.
> 
> > Lastly, it's wrong. ?__NR_SYSCALL_BASE may be 0 or 0x90000 depending on
> > the ABI selected. ?If it's 0x90000, will tracepoint stuff work or will it
> > explode because of a stupidly large table somewhere?
> 
> Right you are.  Didn't consider that.  Does that mean that
> syscall_get_nr() should return the offset from __NR_SYSCALL_BASE, so
> that it will be strictly less than NR_syscalls?

strace sees the 0x90000 offset today, so tracehook is going to be
compatible with our existing APIs, tracehook needs to keep seeing
that offset.

However, I suspect that's incompatible with tracehook as it stands
today - especially when we might be running a kernel with OABI compat
support enabled (which means you'll get 0x90000+ syscalls for OABI and
0+ syscalls for EABI.)

> > kernel/trace/trace_syscalls.c: ? ? ?for (i = 0; i < NR_syscalls; i++) {
> >
> > and
> >
> > kernel/trace/trace_syscalls.c:static DECLARE_BITMAP(enabled_perf_enter_syscalls, NR_syscalls);
> > kernel/trace/trace_syscalls.c:static DECLARE_BITMAP(enabled_perf_exit_syscalls,NR_syscalls);
> >
> > all point at this being very wrong.
> 
> Initially I was going to assign the assembler version of NR_syscalls
> to a symbol which could be referenced from C.  However, putting the C
> declaration for the variable in unistd.h made the assembler unhappy.
> Is it kosher to have C stuff in unistd.h?  Since there was no prior
> example of it, I assumed not.  However, perhaps I could just hide it
> from the assembler with some kind of #ifdef?

#ifndef __ASSEMBLY__

but, as I say, the number of syscalls which the kernel implements is
defined by the size of the table in calls.S, not by the list in
unistd.h.  We may allocate a number and put it in unistd.h to reserve
it before we have a final implementation.