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

[PATCH 0/3] ARM support for syscall tracing

[Wade Farnsworth]

This is a rework of Steven Walter's syscall tracing patches first
posted here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074454.html

I've rebased the patches against the current mainline master branch, as well
as implemented several changes based on feedback from Will Deacon and
Russel King.

Wade Farnsworth (3):
  ARM: add support for the generic syscall.h interface
  ARM: add TRACEHOOK support
  ARM: support syscall tracing

 arch/arm/Kconfig                   |    2 +
 arch/arm/include/asm/ptrace.h      |    5 ++
 arch/arm/include/asm/syscall.h     |   93 ++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/thread_info.h |    2 +
 arch/arm/include/asm/unistd.h      |   12 +++++
 arch/arm/kernel/entry-common.S     |   16 +++++-
 arch/arm/kernel/ptrace.c           |   34 +++++++------
 arch/arm/kernel/signal.c           |    2 +
 8 files changed, 149 insertions(+), 17 deletions(-)
 create mode 100644 arch/arm/include/asm/syscall.h

[PATCH 3/3] ARM: support syscall tracing

[Wade Farnsworth]

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.

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

Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
Signed-off-by: Wade Farnsworth <wade_farnsworth@mentor.com>
---
 arch/arm/Kconfig                   |    1 +
 arch/arm/include/asm/thread_info.h |    2 ++
 arch/arm/include/asm/unistd.h      |   12 ++++++++++++
 arch/arm/kernel/entry-common.S     |   16 ++++++++++++++--
 arch/arm/kernel/ptrace.c           |   23 ++++++++++++++++++-----
 5 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 0faad0c..b1d9396 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
 	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 d4c24d4..f2ff2de 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -146,6 +146,7 @@ extern void vfp_flush_hwstate(struct thread_info *);
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #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)
@@ -156,6 +157,7 @@ extern void vfp_flush_hwstate(struct thread_info *);
 #define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 #define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
 
 /* Checks for any syscall work in entry-common.S */
 #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT)
diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index 512cd14..e4a2e78 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -405,6 +405,18 @@
 #define __NR_process_vm_readv		(__NR_SYSCALL_BASE+376)
 #define __NR_process_vm_writev		(__NR_SYSCALL_BASE+377)
 
+#ifdef __KERNEL__
+
+/* This may need to be greater than __NR_last_syscall+1 in order to
+ * account for the padding in the syscall table */
+#define __NR_syscalls  (380)
+
+#ifndef __ASSEMBLY__
+#define NR_syscalls (__NR_syscalls)
+#endif /* __ASSEMBLY__ */
+
+#endif /* __KERNEL__ */
+
 /*
  * The following SWIs are ARM private.
  */
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 9fd0ba9..9bed212 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -79,6 +79,11 @@ no_work_pending:
 ENDPROC(ret_to_user_from_irq)
 ENDPROC(ret_to_user)
 
+.macro test_syscall_tracing reg
+       tst     \reg, #_TIF_SYSCALL_WORK
+       tsteq   \reg, #_TIF_SYSCALL_TRACEPOINT
+.endm
+
 /*
  * This is how we return from a fork.
  */
@@ -87,7 +92,7 @@ ENTRY(ret_from_fork)
 	get_thread_info tsk
 	ldr	r1, [tsk, #TI_FLAGS]		@ check for syscall tracing
 	mov	why, #1
-	tst	r1, #_TIF_SYSCALL_WORK		@ are we tracing syscalls?
+	test_syscall_tracing r1
 	beq	ret_slow_syscall
 	mov	r1, sp
 	mov	r0, #1				@ trace exit [IP = 1]
@@ -98,6 +103,13 @@ ENDPROC(ret_from_fork)
 	.equ NR_syscalls,0
 #define CALL(x) .equ NR_syscalls,NR_syscalls+1
 #include "calls.S"
+
+/* Ensure that the system call table is not larger than __NR_syscalls,
+ * which is the value the rest of the system sees */
+.ifgt NR_syscalls - __NR_syscalls
+.error "__NR_syscalls is less than the size of the syscall table"
+.endif
+
 #undef CALL
 #define CALL(x) .long x
 
@@ -446,7 +458,7 @@ ENTRY(vector_swi)
 1:
 #endif
 
-	tst	r10, #_TIF_SYSCALL_WORK		@ are we tracing syscalls?
+	test_syscall_tracing r10
 	bne	__sys_trace
 
 	cmp	scno, #NR_syscalls		@ check upper syscall limit
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 5cc5415..5e15e67 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -29,6 +29,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
 /*
@@ -922,15 +925,25 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 		audit_syscall_entry(AUDIT_ARCH_ARMEB, scno, regs->ARM_r0,
 				    regs->ARM_r1, regs->ARM_r2, regs->ARM_r3);
 
-	if (!test_thread_flag(TIF_SYSCALL_TRACE))
+	if (!test_thread_flag(TIF_SYSCALL_TRACE) &&
+	    !test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		return scno;
 
 	current_thread_info()->syscall = scno;
 
-	if (why)
-		tracehook_report_syscall_exit(regs, 0);
-	else if (tracehook_report_syscall_entry(regs))
-		current_thread_info()->syscall = -1;
+	if (test_thread_flag(TIF_SYSCALL_TRACE)) {
+		if (why)
+			tracehook_report_syscall_exit(regs, 0);
+		else if (tracehook_report_syscall_entry(regs))
+			current_thread_info()->syscall = -1;
+	}
+
+	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT)) {
+		if (why)
+			trace_sys_exit(regs, scno);
+		else
+			trace_sys_enter(regs, scno);
+	}
 
 	regs->ARM_ip = ip;
 
-- 
1.7.0.4

[PATCH 3/3] ARM: support syscall tracing

[Will Deacon]

On Wed, Feb 22, 2012 at 02:47:00PM +0000, Wade Farnsworth wrote:
> 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.
> 
> Tests #2 - #4 of "perf test" now complete successfully.
> 
> Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
> Signed-off-by: Wade Farnsworth <wade_farnsworth@mentor.com>
> ---

[...]

> diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
> index 512cd14..e4a2e78 100644
> --- a/arch/arm/include/asm/unistd.h
> +++ b/arch/arm/include/asm/unistd.h
> @@ -405,6 +405,18 @@
>  #define __NR_process_vm_readv		(__NR_SYSCALL_BASE+376)
>  #define __NR_process_vm_writev		(__NR_SYSCALL_BASE+377)
>  
> +#ifdef __KERNEL__
> +
> +/* This may need to be greater than __NR_last_syscall+1 in order to
> + * account for the padding in the syscall table */
> +#define __NR_syscalls  (380)

Do we actually have padding in the syscall table? It looks like a list of
.long to me. I'd rather put the correct number in if possible.

> +#ifndef __ASSEMBLY__
> +#define NR_syscalls (__NR_syscalls)
> +#endif /* __ASSEMBLY__ */

Hmm, these guards feel like a hack. Would moving the define into syscall.h
help?

> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 9fd0ba9..9bed212 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -79,6 +79,11 @@ no_work_pending:
>  ENDPROC(ret_to_user_from_irq)
>  ENDPROC(ret_to_user)
>  
> +.macro test_syscall_tracing reg
> +       tst     \reg, #_TIF_SYSCALL_WORK
> +       tsteq   \reg, #_TIF_SYSCALL_TRACEPOINT
> +.endm
> +
>  /*
>   * This is how we return from a fork.
>   */
> @@ -87,7 +92,7 @@ ENTRY(ret_from_fork)
>  	get_thread_info tsk
>  	ldr	r1, [tsk, #TI_FLAGS]		@ check for syscall tracing
>  	mov	why, #1
> -	tst	r1, #_TIF_SYSCALL_WORK		@ are we tracing syscalls?
> +	test_syscall_tracing r1
>  	beq	ret_slow_syscall
>  	mov	r1, sp
>  	mov	r0, #1				@ trace exit [IP = 1]
> @@ -98,6 +103,13 @@ ENDPROC(ret_from_fork)
>  	.equ NR_syscalls,0
>  #define CALL(x) .equ NR_syscalls,NR_syscalls+1
>  #include "calls.S"
> +
> +/* Ensure that the system call table is not larger than __NR_syscalls,
> + * which is the value the rest of the system sees */
> +.ifgt NR_syscalls - __NR_syscalls
> +.error "__NR_syscalls is less than the size of the syscall table"
> +.endif

I think it would also be nice to check for equality here if we can.

Rest of the code looks alright, but it should be tested with OABI if it
hasn't been already.

Will

[PATCH 3/3] ARM: support syscall tracing

[Wade Farnsworth]

Will Deacon wrote:
> On Wed, Feb 22, 2012 at 02:47:00PM +0000, Wade Farnsworth wrote:
>> 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.
>>
>> Tests #2 - #4 of "perf test" now complete successfully.
>>
>> Signed-off-by: Steven Walter<stevenrwalter@gmail.com>
>> Signed-off-by: Wade Farnsworth<wade_farnsworth@mentor.com>
>> ---
>
> [...]
>
>> diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
>> index 512cd14..e4a2e78 100644
>> --- a/arch/arm/include/asm/unistd.h
>> +++ b/arch/arm/include/asm/unistd.h
>> @@ -405,6 +405,18 @@
>>   #define __NR_process_vm_readv		(__NR_SYSCALL_BASE+376)
>>   #define __NR_process_vm_writev		(__NR_SYSCALL_BASE+377)
>>
>> +#ifdef __KERNEL__
>> +
>> +/* This may need to be greater than __NR_last_syscall+1 in order to
>> + * account for the padding in the syscall table */
>> +#define __NR_syscalls  (380)
>
> Do we actually have padding in the syscall table? It looks like a list of
> .long to me. I'd rather put the correct number in if possible.

This patch will calculate NR_syscalls by counting the number of entries 
in calls.S.  calls.S may add up to three entries of padding at the end 
of that file that __NR_syscalls needs to account for.

>
>> +#ifndef __ASSEMBLY__
>> +#define NR_syscalls (__NR_syscalls)
>> +#endif /* __ASSEMBLY__ */
>
> Hmm, these guards feel like a hack. Would moving the define into syscall.h
> help?

I'll give this a shot, though I'll also note that adding the guards on 
!__ASSEMBLY__ was suggested by Russel:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074506.html

>
>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
>> index 9fd0ba9..9bed212 100644
>> --- a/arch/arm/kernel/entry-common.S
>> +++ b/arch/arm/kernel/entry-common.S
>> @@ -79,6 +79,11 @@ no_work_pending:
>>   ENDPROC(ret_to_user_from_irq)
>>   ENDPROC(ret_to_user)
>>
>> +.macro test_syscall_tracing reg
>> +       tst     \reg, #_TIF_SYSCALL_WORK
>> +       tsteq   \reg, #_TIF_SYSCALL_TRACEPOINT
>> +.endm
>> +
>>   /*
>>    * This is how we return from a fork.
>>    */
>> @@ -87,7 +92,7 @@ ENTRY(ret_from_fork)
>>   	get_thread_info tsk
>>   	ldr	r1, [tsk, #TI_FLAGS]		@ check for syscall tracing
>>   	mov	why, #1
>> -	tst	r1, #_TIF_SYSCALL_WORK		@ are we tracing syscalls?
>> +	test_syscall_tracing r1
>>   	beq	ret_slow_syscall
>>   	mov	r1, sp
>>   	mov	r0, #1				@ trace exit [IP = 1]
>> @@ -98,6 +103,13 @@ ENDPROC(ret_from_fork)
>>   	.equ NR_syscalls,0
>>   #define CALL(x) .equ NR_syscalls,NR_syscalls+1
>>   #include "calls.S"
>> +
>> +/* Ensure that the system call table is not larger than __NR_syscalls,
>> + * which is the value the rest of the system sees */
>> +.ifgt NR_syscalls - __NR_syscalls
>> +.error "__NR_syscalls is less than the size of the syscall table"
>> +.endif
>
> I think it would also be nice to check for equality here if we can.

Should be doable.

>
> Rest of the code looks alright, but it should be tested with OABI if it
> hasn't been already.
>

I might be able to build my userspace against OABI.  I'll see what I can 
dig up.

Thanks for the review!

-Wade