[PATCH v2] ARM: support syscall tracing

[PATCH v2] 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>
---

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

[PATCH v2] ARM: support syscall tracing

[Will Deacon]

Hi Wade,

On Wed, Aug 15, 2012 at 04:14:20PM +0100, Wade Farnsworth wrote:
> 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?

>  	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?

Will

[PATCH v2] ARM: support syscall tracing

[Wade Farnsworth]

Will Deacon wrote:
> Hi Wade,
>
> On Wed, Aug 15, 2012 at 04:14:20PM +0100, Wade Farnsworth wrote:
>> 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?
>
>>   	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

[PATCH v2] ARM: support syscall tracing

[Will Deacon]

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

[PATCH v2] ARM: support syscall tracing

[Wade Farnsworth]

Will Deacon wrote:
> 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.
>

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?

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

[PATCH v2] ARM: support syscall tracing

[Wade Farnsworth]

Wade Farnsworth wrote:
> Will Deacon wrote:
>> 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.
>>
>
> 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.

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

[PATCH v2] ARM: support syscall tracing

[Will Deacon]

On Wed, Aug 15, 2012 at 09:21:35PM +0100, Wade Farnsworth wrote:
> Wade Farnsworth wrote:
> > Will Deacon wrote:
> >> 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.
> >>
> >
> > 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

[PATCH v2] ARM: support syscall tracing

[Wade Farnsworth]

Will Deacon wrote:
> On Wed, Aug 15, 2012 at 09:21:35PM +0100, Wade Farnsworth wrote:
>> Wade Farnsworth wrote:
>>> Will Deacon wrote:
>>>> 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.
>>>>
>>>
>>> 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

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

Thanks,

Wade