ARM: support syscall tracing

arch/arm/kernel/ptrace.c +3 -0

index 3e0fc5f..4e82fc1 100644
@@ -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
 /*

arch/arm/kernel/ptrace.c +0 -0

 	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;
 

Hi Wade,

Quoted 15 lines · Wade Farnsworth

arch/arm/kernel/ptrace.c +3 -0

index 3e0fc5f..4e82fc1 100644
@@ -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
  /*

arch/arm/kernel/ptrace.c static int ptrace_syscall_trace(struct pt_regs *regs, int scno, · line 926

Quoted 7 lines

arch/arm/kernel/ptrace.c +2 -1

@@ -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 5 lines

arch/arm/kernel/ptrace.c +0 -0

  	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;
 

arch/arm/kernel/ptrace.c static int ptrace_syscall_trace(struct pt_regs *regs, int scno, · line 942

Quoted 10 lines

arch/arm/kernel/ptrace.c +6 -4

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

Quoted 2 lines · Wade Farnsworth

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.

Quoted 4 lines

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.

Quoted 9 lines · Will Deacon

Quoted 2 lines · Wade Farnsworth

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

Quoted 4 lines

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

Quoted 11 lines · Wade Farnsworth

Quoted 9 lines · Will Deacon

Quoted 2 lines · Wade Farnsworth

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.

Quoted 9 lines

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.

Quoted 22 lines · Wade Farnsworth

Quoted 11 lines · Wade Farnsworth

Quoted 9 lines · Will Deacon

Quoted 2 lines · Wade Farnsworth

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.

Quoted 9 lines

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

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:

kernel/trace/trace_syscalls.c +3 -3

index 96fc733..bbff120 100644
@@ -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