[PATCH v2] ARM: support syscall tracing

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 <redacted>
Signed-off-by: Wade Farnsworth <redacted>
---

This patch is an update of one I posted some months ago.  It has been
rebased to Linus's current master branch.
- v2: Implemented changes suggested by Will:
  - Folded _TIF_SYSCALL_TRACEPOINT into _TIF_SYSCALL_WORK to save an
    instruction in entry-common.S.
  - Moved calls to trace_sys_{enter,exit} to syscall_trace_{enter,exit}.

 arch/arm/Kconfig                   |    1 +
 arch/arm/include/asm/syscall.h     |    4 ++++
 arch/arm/include/asm/thread_info.h |    4 +++-
 arch/arm/include/asm/unistd.h      |    8 ++++++++
 arch/arm/kernel/entry-common.S     |   11 ++++++++++-
 arch/arm/kernel/ptrace.c           |   20 +++++++++++++++-----
 6 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index e91c7cd..983e9eb 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig

@@ -16,6 +16,7 @@ config ARM
 	select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL
 	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/syscall.h b/arch/arm/include/asm/syscall.h
index c334a23..47486a4 100644
--- a/arch/arm/include/asm/syscall.h
+++ b/arch/arm/include/asm/syscall.h

@@ -9,6 +9,10 @@
 
 #include <linux/err.h>
 
+#include <asm/unistd.h>
+
+#define NR_syscalls (__NR_syscalls)
+
 extern const unsigned long sys_call_table[];
 
 static inline int syscall_get_nr(struct task_struct *task,

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index af7b0bd..5ded667 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h

@@ -148,6 +148,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_SYSCALL_TRACE	8
 #define TIF_SYSCALL_AUDIT	9
+#define TIF_SYSCALL_TRACEPOINT	10
 #define TIF_POLLING_NRFLAG	16
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */

@@ -160,12 +161,13 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
+#define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 
 /* Checks for any syscall work in entry-common.S */
-#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT)
+#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | _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 0cab47d..a566ec2 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h

@@ -406,6 +406,14 @@
 #define __NR_process_vm_writev		(__NR_SYSCALL_BASE+377)
 
 /*
+ * This may need to be greater than __NR_last_syscall+1 in order to
+ * account for the padding in the syscall table
+ */
+#ifdef __KERNEL__
+#define __NR_syscalls  (380)
+#endif /* __KERNEL__ */
+
+/*
  * The following SWIs are ARM private.
  */
 #define __ARM_NR_BASE			(__NR_SYSCALL_BASE+0x0f0000)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 978eac5..b032c94 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S

@@ -94,6 +94,15 @@ 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 equal to __NR_syscalls,
+ * which is the value the rest of the system sees
+ */
+.ifne NR_syscalls - __NR_syscalls
+.error "__NR_syscalls is not equal to the size of the syscall table"
+.endif
+
 #undef CALL
 #define CALL(x) .long x
 

@@ -415,7 +424,7 @@ local_restart:
 1:
 #endif
 
-	tst	r10, #_TIF_SYSCALL_WORK		@ are we tracing syscalls?
+	tst     r10, #_TIF_SYSCALL_WORK		@ are we tracing syscalls?
 	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 3e0fc5f..4e82fc1 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c

@@ -30,6 +30,9 @@
 #include <asm/pgtable.h>
 #include <asm/traps.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/syscalls.h>
+
 #define REG_PC	15
 #define REG_PSR	16
 /*

@@ -918,7 +921,8 @@ static int ptrace_syscall_trace(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;
 
 	current_thread_info()->syscall = scno;

@@ -930,10 +934,12 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
 	ip = regs->ARM_ip;
 	regs->ARM_ip = dir;
 
-	if (dir == PTRACE_SYSCALL_EXIT)
-		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 (dir == PTRACE_SYSCALL_EXIT)
+			tracehook_report_syscall_exit(regs, 0);
+		else if (tracehook_report_syscall_entry(regs))
+			current_thread_info()->syscall = -1;
+	}
 
 	regs->ARM_ip = ip;
 	return current_thread_info()->syscall;

@@ -942,6 +948,8 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
 asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 {
 	int ret = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
+	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
+		trace_sys_enter(regs, ret);
 	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
 			    regs->ARM_r2, regs->ARM_r3);
 	return ret;

@@ -950,6 +958,8 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
 {
 	int ret = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_EXIT);
+	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
+		trace_sys_exit(regs, ret);
 	audit_syscall_exit(regs);
 	return ret;
 }

-- 
1.7.0.4

Hi Wade,

On Wed, Aug 15, 2012 at 04:14:20PM +0100, Wade Farnsworth wrote:

Can we now remove this hunk?

and also this one?

Will

Will Deacon wrote:

Hi Wade,

On Wed, Aug 15, 2012 at 04:14:20PM +0100, Wade Farnsworth wrote:

quoted

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 3e0fc5f..4e82fc1 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c

@@ -30,6 +30,9 @@
  #include<asm/pgtable.h>
  #include<asm/traps.h>

+#define CREATE_TRACE_POINTS
+#include<trace/events/syscalls.h>
+
  #define REG_PC	15
  #define REG_PSR	16
  /*

@@ -918,7 +921,8 @@ static int ptrace_syscall_trace(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;

Can we now remove this hunk?

quoted

  	current_thread_info()->syscall = scno;

@@ -930,10 +934,12 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
  	ip = regs->ARM_ip;
  	regs->ARM_ip = dir;

-	if (dir == PTRACE_SYSCALL_EXIT)
-		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 (dir == PTRACE_SYSCALL_EXIT)
+			tracehook_report_syscall_exit(regs, 0);
+		else if (tracehook_report_syscall_entry(regs))
+			current_thread_info()->syscall = -1;
+	}

and also this one?

We need to set current_thread_info()->syscall, since it's used in the 
call to syscall_get_nr() in perf_syscall_{enter,exit}.

What about moving the setting of ->syscall to 
syscall_trace_{enter,exit}?  That would preserve ptrace_syscall_trace() 
for the TIF_SYSCALL_TRACE case only, but ensure that the field is set 
the TRACEPOINT case as well.  Thoughts?

Thanks for the feedback!

Wade

On Wed, Aug 15, 2012 at 05:58:44PM +0100, Wade Farnsworth wrote:

We need to set current_thread_info()->syscall, since it's used in the 
call to syscall_get_nr() in perf_syscall_{enter,exit}.

Damn. I think that also means we have a bug, given that the SYSCALL_TRACE
code can set this to -1, which gets used as an index into a bitmap by the
looks of it. Considering that we have to pass the syscall number to
trace_sys_enter anyway, it also seems broken.

What about moving the setting of ->syscall to 
syscall_trace_{enter,exit}?  That would preserve ptrace_syscall_trace() 
for the TIF_SYSCALL_TRACE case only, but ensure that the field is set 
the TRACEPOINT case as well.  Thoughts?

I'd be tempted to set the thing unconditionally before checking the thread
flag at the top of ptrace_syscall_trace. This hangs off the slowpath anyway
and it makes everything a lot more readable.

Will

Will Deacon wrote:

On Wed, Aug 15, 2012 at 05:58:44PM +0100, Wade Farnsworth wrote:

quoted

We need to set current_thread_info()->syscall, since it's used in the
call to syscall_get_nr() in perf_syscall_{enter,exit}.

Damn. I think that also means we have a bug, given that the SYSCALL_TRACE
code can set this to -1, which gets used as an index into a bitmap by the
looks of it. Considering that we have to pass the syscall number to
trace_sys_enter anyway, it also seems broken.

I agree.  Looking at the other architectures, it seems the analogous 
function to ptrace_syscall_trace can return -1 under certain 
circumstances, but the original syscall value should be passed onto 
trace_sys_enter and returned from syscall_get_nr().  So, I'm thinking 
that we should modify our behavior accordingly.  What this means for us 
is that we never store -1 in the thread_info syscall field, and then 
pass that into trace_sys_enter instead of the ptrace_syscall_trace 
return value. Do you see any problems with this approach?

quoted

What about moving the setting of ->syscall to
syscall_trace_{enter,exit}?  That would preserve ptrace_syscall_trace()
for the TIF_SYSCALL_TRACE case only, but ensure that the field is set
the TRACEPOINT case as well.  Thoughts?

I'd be tempted to set the thing unconditionally before checking the thread
flag at the top of ptrace_syscall_trace. This hangs off the slowpath anyway
and it makes everything a lot more readable.

OK, I'll make that change.

Wade

Wade Farnsworth wrote:

Will Deacon wrote:

quoted

On Wed, Aug 15, 2012 at 05:58:44PM +0100, Wade Farnsworth wrote:

quoted

We need to set current_thread_info()->syscall, since it's used in the
call to syscall_get_nr() in perf_syscall_{enter,exit}.

Damn. I think that also means we have a bug, given that the SYSCALL_TRACE
code can set this to -1, which gets used as an index into a bitmap by the
looks of it. Considering that we have to pass the syscall number to
trace_sys_enter anyway, it also seems broken.

I agree. Looking at the other architectures, it seems the analogous
function to ptrace_syscall_trace can return -1 under certain
circumstances, but the original syscall value should be passed onto
trace_sys_enter and returned from syscall_get_nr(). So, I'm thinking
that we should modify our behavior accordingly. What this means for us
is that we never store -1 in the thread_info syscall field, and then
pass that into trace_sys_enter instead of the ptrace_syscall_trace
return value. Do you see any problems with this approach?

Hmm, on closer inspection it looks that perf_syscall_enter is broken. 
ftrace_syscall_enter correctly returns if result of a syscall_get_nr is 
negative.  The perf version omits the check for negative values. 
Furthermore, the value of syscall_get_nr() should be -1 if it's not 
called when not in a syscall.  So storing the -1 in the thread_info 
syscall field is not necessarily wrong.  Still, I think the value we 
pass into trace_sys_enter should be the actual syscall number, i.e. the 
value of scno.

quoted

quoted

What about moving the setting of ->syscall to
syscall_trace_{enter,exit}? That would preserve ptrace_syscall_trace()
for the TIF_SYSCALL_TRACE case only, but ensure that the field is set
the TRACEPOINT case as well. Thoughts?

I'd be tempted to set the thing unconditionally before checking the
thread
flag at the top of ptrace_syscall_trace. This hangs off the slowpath
anyway
and it makes everything a lot more readable.

OK, I'll make that change.

Wade

On Wed, Aug 15, 2012 at 09:21:35PM +0100, Wade Farnsworth wrote:

Wade Farnsworth wrote:

quoted

Will Deacon wrote:

quoted

On Wed, Aug 15, 2012 at 05:58:44PM +0100, Wade Farnsworth wrote:

quoted

We need to set current_thread_info()->syscall, since it's used in the
call to syscall_get_nr() in perf_syscall_{enter,exit}.

Damn. I think that also means we have a bug, given that the SYSCALL_TRACE
code can set this to -1, which gets used as an index into a bitmap by the
looks of it. Considering that we have to pass the syscall number to
trace_sys_enter anyway, it also seems broken.

I agree. Looking at the other architectures, it seems the analogous
function to ptrace_syscall_trace can return -1 under certain
circumstances, but the original syscall value should be passed onto
trace_sys_enter and returned from syscall_get_nr(). So, I'm thinking
that we should modify our behavior accordingly. What this means for us
is that we never store -1 in the thread_info syscall field, and then
pass that into trace_sys_enter instead of the ptrace_syscall_trace
return value. Do you see any problems with this approach?

Hmm, on closer inspection it looks that perf_syscall_enter is broken. 
ftrace_syscall_enter correctly returns if result of a syscall_get_nr is 
negative.  The perf version omits the check for negative values. 

Yes, that's what I was getting at with the -1 bitmap index. Something like
this should fix it though:

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 96fc733..bbff120 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c

@@ -506,13 +506,13 @@ static void perf_syscall_enter(void *ignore, struct pt_regs *regs, long id)
        int size;
 
        syscall_nr = syscall_get_nr(current, regs);
-       if (!test_bit(syscall_nr, enabled_perf_enter_syscalls))
-               return;
-
        sys_data = syscall_nr_to_meta(syscall_nr);
        if (!sys_data)
                return;
 
+       if (!test_bit(syscall_nr, enabled_perf_enter_syscalls))
+               return;
+
        /* get the size after alignment with the u32 buffer size field */
        size = sizeof(unsigned long) * sys_data->nb_args + sizeof(*rec);
        size = ALIGN(size + sizeof(u32), sizeof(u64));

If you're happy with that, I can post to LKML and see what people say.

Will

Will Deacon wrote:

I think you have to make a corresponding change in perf_syscall_exit, 
but yes, this looks right to me.

Thanks,

Wade