add syscall tracepoints V3

update FTRACE_SYSCALL_MAX to the current number of syscalls

Signed-off-by Jason Baron <[email protected]>

---
 arch/x86/include/asm/ftrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/include/asm/ftrace.h +0 -0

index bd2c651..7113654 100644

arch/x86/include/asm/ftrace.h +0 -0

 #ifdef CONFIG_FUNCTION_TRACER

Signature · 1 line

1.6.2.5

Layer Frederic's syscall tracer on tracepoints. We create trace events via hooking into the SYCALL_DEFINE macros. This allows us to individually toggle syscall entry and exit points on/off.

Signed-off-by Jason Baron <[email protected]>

---
 include/linux/syscalls.h      |   61 +++++++++++++-
 include/trace/syscall.h       |   18 ++--
 kernel/trace/trace_syscalls.c |  183 ++++++++++++++++++++---------------------
 3 files changed, 159 insertions(+), 103 deletions(-)

include/linux/syscalls.h +5 -0

index 80de700..5e5b4d3 100644
@@ -64,6 +64,7 @@ struct perf_counter_attr;
 #include <linux/sem.h>
 #include <asm/siginfo.h>
 #include <asm/signal.h>
+#include <linux/unistd.h>
 #include <linux/quota.h>
 #include <linux/key.h>
 #include <trace/syscall.h>

@@ -112,6 +113,59 @@ struct perf_counter_attr;
 #define __SC_STR_TDECL5(t, a, ...)	#t, __SC_STR_TDECL4(__VA_ARGS__)
 #define __SC_STR_TDECL6(t, a, ...)	#t, __SC_STR_TDECL5(__VA_ARGS__)
 
+
+#define SYSCALL_TRACE_ENTER_EVENT(sname)				\
+	static struct ftrace_event_call event_enter_##sname;		\
+	static int init_enter_##sname(void)				\

include/linux/syscalls.h +44 -2

+	static struct ftrace_event_call __used				\
+	  __attribute__((__aligned__(4)))				\
+	  __attribute__((section("_ftrace_events")))			\
+	  event_enter_##sname = {					\
+		.name                   = "sys_enter"#sname,		\
+		.system                 = "syscalls",			\
+		.event                  = &event_syscall_enter,		\
+		.raw_init		= init_enter_##sname,		\
+		.regfunc		= reg_event_syscall_enter,	\
+		.unregfunc		= unreg_event_syscall_enter,	\
+		.data			= "sys"#sname,			\
+	}
+
+#define SYSCALL_TRACE_EXIT_EVENT(sname)					\
+	static struct ftrace_event_call event_exit_##sname;		\
+	static int init_exit_##sname(void)				\
+	{								\
+		int num;						\
+		num = syscall_name_to_nr("sys"#sname);			\
+		if (num < 0)						\
+			return -ENOSYS;					\
+		register_ftrace_event(&event_syscall_exit);		\
+		INIT_LIST_HEAD(&event_exit_##sname.fields);		\
+		init_preds(&event_exit_##sname);			\
+		return 0;						\
+	}								\
+	static struct ftrace_event_call __used				\
+	  __attribute__((__aligned__(4)))				\
+	  __attribute__((section("_ftrace_events")))			\
+	  event_exit_##sname = {					\
+		.name                   = "sys_exit"#sname,		\
+		.system                 = "syscalls",			\
+		.event                  = &event_syscall_exit,		\
+		.raw_init		= init_exit_##sname,		\
+		.regfunc		= reg_event_syscall_exit,	\
+		.unregfunc		= unreg_event_syscall_exit,	\
+		.data			= "sys"#sname,			\
+	}
+
 #define SYSCALL_METADATA(sname, nb)				\
 	static const struct syscall_metadata __used		\
 	  __attribute__((__aligned__(4)))			\

@@ -121,7 +175,9 @@ struct perf_counter_attr;
 		.nb_args 	= nb,				\
 		.types		= types_##sname,		\
 		.args		= args_##sname,			\
-	}
+	};							\
+	SYSCALL_TRACE_ENTER_EVENT(sname);			\
+	SYSCALL_TRACE_EXIT_EVENT(sname);
 
 #define SYSCALL_DEFINE0(sname)					\
 	static const struct syscall_metadata __used		\

@@ -131,8 +187,9 @@ struct perf_counter_attr;
 		.name 		= "sys_"#sname,			\
 		.nb_args 	= 0,				\
 	};							\
+	SYSCALL_TRACE_ENTER_EVENT(_##sname);			\
+	SYSCALL_TRACE_EXIT_EVENT(_##sname);			\
 	asmlinkage long sys_##sname(void)
-
 #else
 #define SYSCALL_DEFINE0(name)	   asmlinkage long sys_##name(void)
 #endif

include/trace/syscall.h +9 -9

index 3951d77..73fb8b4 100644
@@ -2,6 +2,8 @@
 #define _TRACE_SYSCALL_H
 
 #include <linux/tracepoint.h>
+#include <linux/unistd.h>
+#include <linux/ftrace_event.h>
 
 #include <asm/ptrace.h>
 

@@ -40,15 +42,13 @@ struct syscall_metadata {
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 extern struct syscall_metadata *syscall_nr_to_meta(int nr);
-extern void start_ftrace_syscalls(void);
-extern void stop_ftrace_syscalls(void);
-extern void ftrace_syscall_enter(struct pt_regs *regs);
-extern void ftrace_syscall_exit(struct pt_regs *regs);
-#else
-static inline void start_ftrace_syscalls(void)			{ }
-static inline void stop_ftrace_syscalls(void)			{ }
-static inline void ftrace_syscall_enter(struct pt_regs *regs)	{ }
-static inline void ftrace_syscall_exit(struct pt_regs *regs)	{ }
+extern int syscall_name_to_nr(char *name);
+extern struct trace_event event_syscall_enter;
+extern struct trace_event event_syscall_exit;
+extern int reg_event_syscall_enter(void *ptr);
+extern void unreg_event_syscall_enter(void *ptr);
+extern int reg_event_syscall_exit(void *ptr);
+extern void unreg_event_syscall_exit(void *ptr);
 #endif
 
 #endif /* _TRACE_SYSCALL_H */

kernel/trace/trace_syscalls.c +12 -53

index 08aed43..c7ae25e 100644
@@ -1,15 +1,16 @@
 #include <trace/syscall.h>
 #include <linux/kernel.h>
+#include <linux/ftrace.h>
 #include <asm/syscall.h>
 
 #include "trace_output.h"
 #include "trace.h"
 
-/* Keep a counter of the syscall tracing users */
-static int refcount;
-
-/* Prevent from races on thread flags toggling */
 static DEFINE_MUTEX(syscall_trace_lock);
+static int sys_refcount_enter;
+static int sys_refcount_exit;
+static DECLARE_BITMAP(enabled_enter_syscalls, FTRACE_SYSCALL_MAX);
+static DECLARE_BITMAP(enabled_exit_syscalls, FTRACE_SYSCALL_MAX);
 
 /* Option to display the parameters types */
 enum {

@@ -95,53 +96,7 @@ print_syscall_exit(struct trace_iterator *iter, int flags)
 	return TRACE_TYPE_HANDLED;
 }
 
-void start_ftrace_syscalls(void)
-{
-	unsigned long flags;
-	struct task_struct *g, *t;
-
-	mutex_lock(&syscall_trace_lock);
-
-	/* Don't enable the flag on the tasks twice */
-	if (++refcount != 1)
-		goto unlock;
-
-	read_lock_irqsave(&tasklist_lock, flags);
-
-	do_each_thread(g, t) {
-		set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
-	} while_each_thread(g, t);
-
-	read_unlock_irqrestore(&tasklist_lock, flags);
-
-unlock:
-	mutex_unlock(&syscall_trace_lock);
-}
-
-void stop_ftrace_syscalls(void)
-{
-	unsigned long flags;
-	struct task_struct *g, *t;
-
-	mutex_lock(&syscall_trace_lock);
-
-	/* There are perhaps still some users */
-	if (--refcount)
-		goto unlock;
-
-	read_lock_irqsave(&tasklist_lock, flags);
-
-	do_each_thread(g, t) {
-		clear_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
-	} while_each_thread(g, t);
-
-	read_unlock_irqrestore(&tasklist_lock, flags);
-
-unlock:
-	mutex_unlock(&syscall_trace_lock);
-}
-
-void ftrace_syscall_enter(struct pt_regs *regs)
+void ftrace_syscall_enter(struct pt_regs *regs, long id)
 {
 	struct syscall_trace_enter *entry;
 	struct syscall_metadata *sys_data;

@@ -150,6 +105,8 @@ void ftrace_syscall_enter(struct pt_regs *regs)
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_enter_syscalls))
+		return;
 
 	sys_data = syscall_nr_to_meta(syscall_nr);
 	if (!sys_data)

@@ -170,7 +127,7 @@ void ftrace_syscall_enter(struct pt_regs *regs)
 	trace_wake_up();
 }
 
-void ftrace_syscall_exit(struct pt_regs *regs)
+void ftrace_syscall_exit(struct pt_regs *regs, long ret)
 {
 	struct syscall_trace_exit *entry;
 	struct syscall_metadata *sys_data;

@@ -178,6 +135,8 @@ void ftrace_syscall_exit(struct pt_regs *regs)
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_exit_syscalls))
+		return;
 
 	sys_data = syscall_nr_to_meta(syscall_nr);
 	if (!sys_data)

@@ -196,54 +155,94 @@ void ftrace_syscall_exit(struct pt_regs *regs)
 	trace_wake_up();
 }
 
-static int init_syscall_tracer(struct trace_array *tr)
+int reg_event_syscall_enter(void *ptr)
 {

kernel/trace/trace_syscalls.c +72 -36

+		return -ENOSYS;
+	mutex_lock(&syscall_trace_lock);
+	if (!sys_refcount_enter)
+		ret = register_trace_syscall_enter(ftrace_syscall_enter);
+	if (ret) {
+		pr_info("event trace: Could not activate"
+				"syscall entry trace point");
+	} else {
+		set_bit(num, enabled_enter_syscalls);
+		sys_refcount_enter++;
+	}
+	mutex_unlock(&syscall_trace_lock);
+	return ret;
 }
 
-static void reset_syscall_tracer(struct trace_array *tr)
+void unreg_event_syscall_enter(void *ptr)
 {
-	stop_ftrace_syscalls();
-	tracing_reset_online_cpus(tr);
-}
-
-static struct trace_event syscall_enter_event = {
-	.type	 	= TRACE_SYSCALL_ENTER,
-	.trace		= print_syscall_enter,
-};
-
-static struct trace_event syscall_exit_event = {
-	.type	 	= TRACE_SYSCALL_EXIT,
-	.trace		= print_syscall_exit,
-};
+	int num;
+	char *name;
 
-static struct tracer syscall_tracer __read_mostly = {
-	.name	     	= "syscall",
-	.init		= init_syscall_tracer,
-	.reset		= reset_syscall_tracer,
-	.flags		= &syscalls_flags,
-};
+	name = (char *)ptr;
+	num = syscall_name_to_nr(name);
+	if (num < 0 || num >= FTRACE_SYSCALL_MAX)
+		return;
+	mutex_lock(&syscall_trace_lock);
+	sys_refcount_enter--;
+	clear_bit(num, enabled_enter_syscalls);
+	if (!sys_refcount_enter)
+		unregister_trace_syscall_enter(ftrace_syscall_enter);
+	mutex_unlock(&syscall_trace_lock);
+}
 
-__init int register_ftrace_syscalls(void)
+int reg_event_syscall_exit(void *ptr)
 {
-	int ret;
-
-	ret = register_ftrace_event(&syscall_enter_event);
-	if (!ret) {
-		printk(KERN_WARNING "event %d failed to register\n",
-		       syscall_enter_event.type);
-		WARN_ON_ONCE(1);
+	int ret = 0;
+	int num;
+	char *name;
+
+	name = (char *)ptr;
+	num = syscall_name_to_nr(name);
+	if (num < 0 || num >= FTRACE_SYSCALL_MAX)
+		return -ENOSYS;
+	mutex_lock(&syscall_trace_lock);
+	if (!sys_refcount_exit)
+		ret = register_trace_syscall_exit(ftrace_syscall_exit);
+	if (ret) {
+		pr_info("event trace: Could not activate"
+				"syscall exit trace point");
+	} else {
+		set_bit(num, enabled_exit_syscalls);
+		sys_refcount_exit++;
 	}
+	mutex_unlock(&syscall_trace_lock);
+	return ret;
+}
 
-	ret = register_ftrace_event(&syscall_exit_event);
-	if (!ret) {
-		printk(KERN_WARNING "event %d failed to register\n",
-		       syscall_exit_event.type);
-		WARN_ON_ONCE(1);
-	}
+void unreg_event_syscall_exit(void *ptr)
+{
+	int num;
+	char *name;
 
-	return register_tracer(&syscall_tracer);
+	name = (char *)ptr;
+	num = syscall_name_to_nr(name);
+	if (num < 0 || num >= FTRACE_SYSCALL_MAX)
+		return;
+	mutex_lock(&syscall_trace_lock);
+	sys_refcount_exit--;
+	clear_bit(num, enabled_exit_syscalls);
+	if (!sys_refcount_exit)
+		unregister_trace_syscall_exit(ftrace_syscall_exit);
+	mutex_unlock(&syscall_trace_lock);
 }
-device_initcall(register_ftrace_syscalls);
+
+struct trace_event event_syscall_enter = {
+	.trace			= print_syscall_enter,
+	.type			= TRACE_SYSCALL_ENTER
+};
+
+struct trace_event event_syscall_exit = {
+	.trace			= print_syscall_exit,
+	.type			= TRACE_SYSCALL_EXIT
+};

Signature · 1 line

1.6.2.5

Quoted 15 lines · Frederic Weisbecker

Quoted 12 lines

kernel/trace/trace_syscalls.c +9 -0

+struct trace_event event_syscall_enter = {
+	.trace			= print_syscall_enter,
+	.type			= TRACE_SYSCALL_ENTER
+};
+
+struct trace_event event_syscall_exit = {
+	.trace			= print_syscall_exit,
+	.type			= TRACE_SYSCALL_EXIT
+};

Signature · 1 line

1.6.2.5

Nice.

Quoted 5 lines

It's a bit too bad that enter and exit must be that separated whereas their callbacks are pretty the same.

But I guess if we want to nicely decouple both, we don't have the choice.

Yeah - and enter and exit are different, in terms of state.

One thing that would be nice in the future (as an add-on - this patch-set looks useful already) is to allow the sampling of user register state as well via these tracepoints. That way we'd have a much faster (and completely transparent) implementation of strace in essence, with unique features such as system-wide or per cpu strace-ing.

Ingo

add an optional * void pointer to 'ftrace_event_call' that is passed in for regfunc and unregfunc.

Signed-off-by Jason Baron <[email protected]>

---
 include/linux/ftrace_event.h |    5 +++--
 include/trace/ftrace.h       |    4 ++--
 kernel/trace/trace_events.c  |    4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

include/linux/ftrace_event.h +3 -2

index ac8c6f8..8544f12 100644
@@ -112,8 +112,8 @@ struct ftrace_event_call {
 	struct dentry		*dir;
 	struct trace_event	*event;
 	int			enabled;
-	int			(*regfunc)(void);
-	void			(*unregfunc)(void);
+	int			(*regfunc)(void *);
+	void			(*unregfunc)(void *);
 	int			id;
 	int			(*raw_init)(void);
 	int			(*show_format)(struct trace_seq *s);

@@ -122,6 +122,7 @@ struct ftrace_event_call {
 	int			filter_active;
 	struct event_filter	*filter;
 	void			*mod;
+	void			*data;
 
 	atomic_t		profile_count;
 	int			(*profile_enable)(struct ftrace_event_call *);

include/trace/ftrace.h +0 -0

index 80e5f6c..a0de384 100644

include/trace/ftrace.h +0 -0

 {									\
 	unregister_trace_##call(ftrace_raw_event_##call);		\
 }									\

kernel/trace/trace_events.c +2 -2

index f95f847..1d289e2 100644
@@ -86,14 +86,14 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
 		if (call->enabled) {
 			call->enabled = 0;
 			tracing_stop_cmdline_record();
-			call->unregfunc();
+			call->unregfunc(call->data);
 		}
 		break;
 	case 1:
 		if (!call->enabled) {
 			call->enabled = 1;
 			tracing_start_cmdline_record();
-			call->regfunc();
+			call->regfunc(call->data);
 		}
 		break;
 	}

Signature · 1 line

1.6.2.5

Make 'perf stat -e syscalls:sys_enter_blah' work with syscall style tracepoints.

Signed-off-by Jason Baron <[email protected]>

---
 include/linux/perf_counter.h  |    2 +
 include/linux/syscalls.h      |   52 +++++++++++++++++-
 include/trace/syscall.h       |    7 +++
 kernel/trace/trace_syscalls.c |  121 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 181 insertions(+), 1 deletions(-)

include/linux/perf_counter.h +2 -0

index c484834..aaf0c74 100644
@@ -734,6 +734,8 @@ extern int sysctl_perf_counter_mlock;
 extern int sysctl_perf_counter_sample_rate;
 
 extern void perf_counter_init(void);
+extern void perf_tpcounter_event(int event_id, u64 addr, u64 count,
+				 void *record, int entry_size);
 
 #ifndef perf_misc_flags
 #define perf_misc_flags(regs)	(user_mode(regs) ? PERF_EVENT_MISC_USER : \

include/linux/syscalls.h +51 -1

index ce4b01c..5541e75 100644
@@ -98,6 +98,53 @@ struct perf_counter_attr;
 #define __SC_TEST5(t5, a5, ...)	__SC_TEST(t5); __SC_TEST4(__VA_ARGS__)
 #define __SC_TEST6(t6, a6, ...)	__SC_TEST(t6); __SC_TEST5(__VA_ARGS__)
 
+#ifdef CONFIG_EVENT_PROFILE
+#define TRACE_SYS_ENTER_PROFILE(sname)					       \
+static int prof_sysenter_enable_##sname(struct ftrace_event_call *event_call)  \
+{									       \
+	int ret = 0;							       \
+	if (!atomic_inc_return(&event_enter_##sname.profile_count))	       \
+		ret = reg_prof_syscall_enter("sys"#sname);		       \
+	return ret;							       \
+}									       \
+									       \
+static void prof_sysenter_disable_##sname(struct ftrace_event_call *event_call)\
+{									       \
+	if (atomic_add_negative(-1, &event_enter_##sname.profile_count))       \
+		unreg_prof_syscall_enter("sys"#sname);			       \
+}
+
+#define TRACE_SYS_EXIT_PROFILE(sname)					       \
+static int prof_sysexit_enable_##sname(struct ftrace_event_call *event_call)   \
+{									       \
+	int ret = 0;							       \
+	if (!atomic_inc_return(&event_exit_##sname.profile_count))	       \
+		ret = reg_prof_syscall_exit("sys"#sname);		       \
+	return ret;							       \
+}									       \
+									       \
+static void prof_sysexit_disable_##sname(struct ftrace_event_call *event_call) \
+{                                                                              \
+	if (atomic_add_negative(-1, &event_exit_##sname.profile_count))	       \
+		unreg_prof_syscall_exit("sys"#sname);			       \
+}
+
+#define TRACE_SYS_ENTER_PROFILE_INIT(sname)				       \
+	.profile_count = ATOMIC_INIT(-1),				       \
+	.profile_enable = prof_sysenter_enable_##sname,			       \
+	.profile_disable = prof_sysenter_disable_##sname,
+
+#define TRACE_SYS_EXIT_PROFILE_INIT(sname)				       \
+	.profile_count = ATOMIC_INIT(-1),				       \
+	.profile_enable = prof_sysexit_enable_##sname,			       \
+	.profile_disable = prof_sysexit_disable_##sname,
+#else
+#define TRACE_SYS_ENTER_PROFILE(sname)
+#define TRACE_SYS_ENTER_PROFILE_INIT(sname)
+#define TRACE_SYS_EXIT_PROFILE(sname)
+#define TRACE_SYS_EXIT_PROFILE_INIT(sname)
+#endif
+
 #ifdef CONFIG_FTRACE_SYSCALLS
 #define __SC_STR_ADECL1(t, a)		#a
 #define __SC_STR_ADECL2(t, a, ...)	#a, __SC_STR_ADECL1(__VA_ARGS__)

@@ -113,7 +160,6 @@ struct perf_counter_attr;
 #define __SC_STR_TDECL5(t, a, ...)	#t, __SC_STR_TDECL4(__VA_ARGS__)
 #define __SC_STR_TDECL6(t, a, ...)	#t, __SC_STR_TDECL5(__VA_ARGS__)
 
-
 #define SYSCALL_TRACE_ENTER_EVENT(sname)				\
 	static struct ftrace_event_call event_enter_##sname;		\
 	struct trace_event enter_syscall_print_##sname = {		\

@@ -134,6 +180,7 @@ struct perf_counter_attr;
 		init_preds(&event_enter_##sname);			\
 		return 0;						\
 	}								\
+	TRACE_SYS_ENTER_PROFILE(sname);					\
 	static struct ftrace_event_call __used				\
 	  __attribute__((__aligned__(4)))				\
 	  __attribute__((section("_ftrace_events")))			\

@@ -145,6 +192,7 @@ struct perf_counter_attr;
 		.regfunc		= reg_event_syscall_enter,	\
 		.unregfunc		= unreg_event_syscall_enter,	\
 		.data			= "sys"#sname,			\
+		TRACE_SYS_ENTER_PROFILE_INIT(sname)			\
 	}
 
 #define SYSCALL_TRACE_EXIT_EVENT(sname)					\

@@ -167,6 +215,7 @@ struct perf_counter_attr;
 		init_preds(&event_exit_##sname);			\
 		return 0;						\
 	}								\
+	TRACE_SYS_EXIT_PROFILE(sname);					\
 	static struct ftrace_event_call __used				\
 	  __attribute__((__aligned__(4)))				\
 	  __attribute__((section("_ftrace_events")))			\

@@ -178,6 +227,7 @@ struct perf_counter_attr;
 		.regfunc		= reg_event_syscall_exit,	\
 		.unregfunc		= unreg_event_syscall_exit,	\
 		.data			= "sys"#sname,			\
+		TRACE_SYS_EXIT_PROFILE_INIT(sname)			\
 	}
 
 #define SYSCALL_METADATA(sname, nb)				\

include/trace/syscall.h +7 -0

index df62840..3ab6dd1 100644
@@ -58,5 +58,12 @@ extern void unreg_event_syscall_exit(void *ptr);
 enum print_line_t print_syscall_enter(struct trace_iterator *iter, int flags);
 enum print_line_t print_syscall_exit(struct trace_iterator *iter, int flags);
 #endif
+#ifdef CONFIG_EVENT_PROFILE
+int reg_prof_syscall_enter(char *name);
+void unreg_prof_syscall_enter(char *name);
+int reg_prof_syscall_exit(char *name);
+void unreg_prof_syscall_exit(char *name);
+
+#endif
 
 #endif /* _TRACE_SYSCALL_H */

kernel/trace/trace_syscalls.c +121 -0

index e58a9c1..f4eaec3 100644
@@ -1,6 +1,7 @@
 #include <trace/syscall.h>
 #include <linux/kernel.h>
 #include <linux/ftrace.h>
+#include <linux/perf_counter.h>
 #include <asm/syscall.h>
 
 #include "trace_output.h"

@@ -252,3 +253,123 @@ struct trace_event event_syscall_enter = {
 struct trace_event event_syscall_exit = {
 	.trace			= print_syscall_exit,
 };
+
+#ifdef CONFIG_EVENT_PROFILE
+static DECLARE_BITMAP(enabled_prof_enter_syscalls, FTRACE_SYSCALL_MAX);
+static DECLARE_BITMAP(enabled_prof_exit_syscalls, FTRACE_SYSCALL_MAX);
+static int sys_prof_refcount_enter;
+static int sys_prof_refcount_exit;
+
+static void prof_syscall_enter(struct pt_regs *regs, long id)
+{
+	struct syscall_metadata *sys_data;
+	int syscall_nr;
+
+	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_prof_enter_syscalls))
+		return;
+
+	sys_data = syscall_nr_to_meta(syscall_nr);
+	if (!sys_data)
+		return;
+
+	perf_tpcounter_event(sys_data->enter_id, 0, 1, NULL, 0);
+}
+
+int reg_prof_syscall_enter(char *name)
+{
+	int ret = 0;
+	int num;
+
+	num = syscall_name_to_nr(name);
+	if (num < 0 || num >= FTRACE_SYSCALL_MAX)
+		return -ENOSYS;
+
+	mutex_lock(&syscall_trace_lock);
+	if (!sys_prof_refcount_enter)
+		ret = register_trace_syscall_enter(prof_syscall_enter);
+	if (ret) {
+		pr_info("event trace: Could not activate"
+				"syscall entry trace point");
+	} else {
+		set_bit(num, enabled_prof_enter_syscalls);
+		sys_prof_refcount_enter++;
+	}
+	mutex_unlock(&syscall_trace_lock);
+	return ret;
+}
+
+void unreg_prof_syscall_enter(char *name)
+{
+	int num;
+
+	num = syscall_name_to_nr(name);
+	if (num < 0 || num >= FTRACE_SYSCALL_MAX)
+		return;
+
+	mutex_lock(&syscall_trace_lock);
+	sys_prof_refcount_enter--;
+	clear_bit(num, enabled_prof_enter_syscalls);
+	if (!sys_prof_refcount_enter)
+		unregister_trace_syscall_enter(prof_syscall_enter);
+	mutex_unlock(&syscall_trace_lock);
+}
+
+static void prof_syscall_exit(struct pt_regs *regs, long ret)
+{
+	struct syscall_metadata *sys_data;
+	int syscall_nr;
+
+	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_prof_exit_syscalls))
+		return;
+
+	sys_data = syscall_nr_to_meta(syscall_nr);
+	if (!sys_data)
+		return;
+
+	perf_tpcounter_event(sys_data->exit_id, 0, 1, NULL, 0);
+}
+
+int reg_prof_syscall_exit(char *name)
+{
+	int ret = 0;
+	int num;
+
+	num = syscall_name_to_nr(name);
+	if (num < 0 || num >= FTRACE_SYSCALL_MAX)
+		return -ENOSYS;
+
+	mutex_lock(&syscall_trace_lock);
+	if (!sys_prof_refcount_exit)
+		ret = register_trace_syscall_exit(prof_syscall_exit);
+	if (ret) {
+		pr_info("event trace: Could not activate"
+				"syscall entry trace point");
+	} else {
+		set_bit(num, enabled_prof_exit_syscalls);
+		sys_prof_refcount_exit++;
+	}
+	mutex_unlock(&syscall_trace_lock);
+	return ret;
+}
+
+void unreg_prof_syscall_exit(char *name)
+{
+	int num;
+
+	num = syscall_name_to_nr(name);
+	if (num < 0 || num >= FTRACE_SYSCALL_MAX)
+		return;
+
+	mutex_lock(&syscall_trace_lock);
+	sys_prof_refcount_exit--;
+	clear_bit(num, enabled_prof_exit_syscalls);
+	if (!sys_prof_refcount_exit)
+		unregister_trace_syscall_exit(prof_syscall_exit);
+	mutex_unlock(&syscall_trace_lock);
+}
+
+#endif
+
+

Signature · 1 line

1.6.2.5

Quoted 1 line · Jason Baron

Make 'perf stat -e syscalls:sys_enter_blah' work with syscall style tracepoints.

It would be nice to also be able to type:

perf stat -e syscalls:blah

and then having both enter/exit counters.

Quoted 9 lines · Frederic Weisbecker

Quoted 1 line · Jason Baron

Make 'perf stat -e syscalls:sys_enter_blah' work with syscall style tracepoints.

It would be nice to also be able to type:

perf stat -e syscalls:blah

and then having both enter/exit counters.

Plus wildcard/regex support would be nice as well in the listing of events. That way one could do a shortcut of:

perf stat -e syscalls:*

to trace all syscalls. Or:

perf stat -e syscalls:*read*

to see all the read variants - etc.

Ingo

This patch associates an id with each syscall trace event, so that we can identify each syscall trace event using the 'perf' tool.

Signed-off-by Jason Baron <[email protected]>

---
 arch/x86/kernel/ftrace.c      |   10 ++++++++++
 include/linux/syscalls.h      |   22 ++++++++++++++++++----
 include/trace/syscall.h       |    8 ++++++++
 kernel/trace/trace.h          |    6 ------
 kernel/trace/trace_syscalls.c |   26 ++++++++++++++++----------
 5 files changed, 52 insertions(+), 20 deletions(-)

arch/x86/kernel/ftrace.c +10 -0

index 0d93d40..3cff121 100644
@@ -516,6 +516,16 @@ int syscall_name_to_nr(char *name)
 	return -1;
 }
 
+void set_syscall_enter_id(int num, int id)
+{
+	syscalls_metadata[num]->enter_id = id;
+}
+
+void set_syscall_exit_id(int num, int id)
+{
+	syscalls_metadata[num]->exit_id = id;
+}
+
 static int __init arch_init_ftrace_syscalls(void)
 {
 	int i;

include/linux/syscalls.h +2 -0

index 5e5b4d3..ce4b01c 100644
@@ -116,13 +116,20 @@ struct perf_counter_attr;
 
 #define SYSCALL_TRACE_ENTER_EVENT(sname)				\
 	static struct ftrace_event_call event_enter_##sname;		\
+	struct trace_event enter_syscall_print_##sname = {		\
+		.trace                  = print_syscall_enter,		\

include/linux/syscalls.h +13 -2

+		if (!id)						\
+			return -ENODEV;					\
+		event_enter_##sname.id = id;				\
+		set_syscall_enter_id(num, id);				\
 		INIT_LIST_HEAD(&event_enter_##sname.fields);		\
 		init_preds(&event_enter_##sname);			\
 		return 0;						\

@@ -142,13 +149,20 @@ struct perf_counter_attr;
 
 #define SYSCALL_TRACE_EXIT_EVENT(sname)					\
 	static struct ftrace_event_call event_exit_##sname;		\
+	struct trace_event exit_syscall_print_##sname = {		\
+		.trace                  = print_syscall_exit,		\
+	};								\
 	static int init_exit_##sname(void)				\
 	{								\
-		int num;						\
+		int num, id;						\
 		num = syscall_name_to_nr("sys"#sname);			\
 		if (num < 0)						\
 			return -ENOSYS;					\
-		register_ftrace_event(&event_syscall_exit);		\
+		id = register_ftrace_event(&exit_syscall_print_##sname);\
+		if (!id)						\
+			return -ENODEV;					\
+		event_exit_##sname.id = id;				\
+		set_syscall_exit_id(num, id);				\
 		INIT_LIST_HEAD(&event_exit_##sname.fields);		\
 		init_preds(&event_exit_##sname);			\
 		return 0;						\

include/trace/syscall.h +8 -0

index 73fb8b4..df62840 100644
@@ -32,23 +32,31 @@ DECLARE_TRACE_WITH_CALLBACK(syscall_exit,
  * @nb_args: number of parameters it takes
  * @types: list of types as strings
  * @args: list of args as strings (args[i] matches types[i])
+ * @enter_id: associated ftrace enter event id
+ * @exit_id: associated ftrace exit event id
  */
 struct syscall_metadata {
 	const char	*name;
 	int		nb_args;
 	const char	**types;
 	const char	**args;
+	int		enter_id;
+	int		exit_id;
 };
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 extern struct syscall_metadata *syscall_nr_to_meta(int nr);
 extern int syscall_name_to_nr(char *name);
+void set_syscall_enter_id(int num, int id);
+void set_syscall_exit_id(int num, int id);
 extern struct trace_event event_syscall_enter;
 extern struct trace_event event_syscall_exit;
 extern int reg_event_syscall_enter(void *ptr);
 extern void unreg_event_syscall_enter(void *ptr);
 extern int reg_event_syscall_exit(void *ptr);
 extern void unreg_event_syscall_exit(void *ptr);
+enum print_line_t print_syscall_enter(struct trace_iterator *iter, int flags);
+enum print_line_t print_syscall_exit(struct trace_iterator *iter, int flags);
 #endif
 
 #endif /* _TRACE_SYSCALL_H */

kernel/trace/trace.h +0 -6

index 44308f3..606073c 100644
@@ -38,8 +38,6 @@ enum trace_type {
 	TRACE_GRAPH_ENT,
 	TRACE_USER_STACK,
 	TRACE_HW_BRANCHES,
-	TRACE_SYSCALL_ENTER,
-	TRACE_SYSCALL_EXIT,
 	TRACE_KMEM_ALLOC,
 	TRACE_KMEM_FREE,
 	TRACE_POWER,

@@ -334,10 +332,6 @@ extern void __ftrace_bad_type(void);
 			  TRACE_KMEM_ALLOC);	\
 		IF_ASSIGN(var, ent, struct kmemtrace_free_entry,	\
 			  TRACE_KMEM_FREE);	\
-		IF_ASSIGN(var, ent, struct syscall_trace_enter,		\
-			  TRACE_SYSCALL_ENTER);				\
-		IF_ASSIGN(var, ent, struct syscall_trace_exit,		\
-			  TRACE_SYSCALL_EXIT);				\
 		IF_ASSIGN(var, ent, struct ksym_trace_entry, TRACE_KSYM);\
 		__ftrace_bad_type();					\
 	} while (0)

kernel/trace/trace_syscalls.c +16 -8

index c7ae25e..e58a9c1 100644
@@ -36,14 +36,18 @@ print_syscall_enter(struct trace_iterator *iter, int flags)
 	struct syscall_metadata *entry;
 	int i, ret, syscall;
 
-	trace_assign_type(trace, ent);
-
+	trace = (typeof(trace))ent;
 	syscall = trace->nr;
-
 	entry = syscall_nr_to_meta(syscall);
+
 	if (!entry)
 		goto end;
 
+	if (entry->enter_id != ent->type) {
+		WARN_ON_ONCE(1);
+		goto end;
+	}
+
 	ret = trace_seq_printf(s, "%s(", entry->name);
 	if (!ret)
 		return TRACE_TYPE_PARTIAL_LINE;

@@ -78,16 +82,20 @@ print_syscall_exit(struct trace_iterator *iter, int flags)
 	struct syscall_metadata *entry;
 	int ret;
 
-	trace_assign_type(trace, ent);
-
+	trace = (typeof(trace))ent;
 	syscall = trace->nr;
-
 	entry = syscall_nr_to_meta(syscall);
+
 	if (!entry) {
 		trace_seq_printf(s, "\n");
 		return TRACE_TYPE_HANDLED;
 	}
 
+	if (entry->exit_id != ent->type) {
+		WARN_ON_ONCE(1);
+		return TRACE_TYPE_UNHANDLED;
+	}
+
 	ret = trace_seq_printf(s, "%s -> 0x%lx\n", entry->name,
 				trace->ret);
 	if (!ret)

@@ -114,7 +122,7 @@ void ftrace_syscall_enter(struct pt_regs *regs, long id)
 
 	size = sizeof(*entry) + sizeof(unsigned long) * sys_data->nb_args;
 
-	event = trace_current_buffer_lock_reserve(TRACE_SYSCALL_ENTER, size,
+	event = trace_current_buffer_lock_reserve(sys_data->enter_id, size,
 							0, 0);
 	if (!event)
 		return;

@@ -142,7 +150,7 @@ void ftrace_syscall_exit(struct pt_regs *regs, long ret)
 	if (!sys_data)
 		return;
 
-	event = trace_current_buffer_lock_reserve(TRACE_SYSCALL_EXIT,
+	event = trace_current_buffer_lock_reserve(sys_data->exit_id,
 				sizeof(*entry), 0, 0);
 	if (!event)
 		return;

Signature · 1 line

1.6.2.5

Quoted 12 lines

kernel/trace/trace_syscalls.c +9 -0

+struct trace_event event_syscall_enter = {
+	.trace			= print_syscall_enter,
+	.type			= TRACE_SYSCALL_ENTER
+};
+
+struct trace_event event_syscall_exit = {
+	.trace			= print_syscall_exit,
+	.type			= TRACE_SYSCALL_EXIT
+};

Signature · 1 line

1.6.2.5

Nice.

It's a bit too bad that enter and exit must be that separated whereas their callbacks are pretty the same.

But I guess if we want to nicely decouple both, we don't have the choice.

Quoted 14 lines · Jason Baron

update FTRACE_SYSCALL_MAX to the current number of syscalls

Signed-off-by Jason Baron <[email protected]>

---
 arch/x86/include/asm/ftrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/include/asm/ftrace.h +0 -0

index bd2c651..7113654 100644
@@ -30,9 +30,9 @@
 

arch/x86/include/asm/ftrace.h line 36

Quoted 8 lines

arch/x86/include/asm/ftrace.h +2 -2

 /* FIXME: I don't want to stay hardcoded */
 #ifdef CONFIG_X86_64
-# define FTRACE_SYSCALL_MAX     296
+# define FTRACE_SYSCALL_MAX     299
 #else
-# define FTRACE_SYSCALL_MAX     333
+# define FTRACE_SYSCALL_MAX     337
 #endif

I don't remember why we had to use a hardcoded number. Is there no way to keep being sync with the current number of syscalls? We dwant to avoid patching the kernel each time we have a new syscall :-)

Quoted 32 lines · Jason Baron

add an optional * void pointer to 'ftrace_event_call' that is passed in for regfunc and unregfunc.

Signed-off-by Jason Baron <[email protected]>

---
 include/linux/ftrace_event.h |    5 +++--
 include/trace/ftrace.h       |    4 ++--
 kernel/trace/trace_events.c  |    4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

include/linux/ftrace_event.h +3 -2

index ac8c6f8..8544f12 100644
@@ -112,8 +112,8 @@ struct ftrace_event_call {
 	struct dentry		*dir;
 	struct trace_event	*event;
 	int			enabled;
-	int			(*regfunc)(void);
-	void			(*unregfunc)(void);
+	int			(*regfunc)(void *);
+	void			(*unregfunc)(void *);
 	int			id;
 	int			(*raw_init)(void);
 	int			(*show_format)(struct trace_seq *s);

@@ -122,6 +122,7 @@ struct ftrace_event_call {
 	int			filter_active;
 	struct event_filter	*filter;
 	void			*mod;
+	void			*data;
 

include/trace/ftrace.h static void ftrace_raw_event_##call(proto) \ · line 571

Quoted 12 lines

include/linux/ftrace_event.h +0 -0

 	atomic_t		profile_count;
 	int			(*profile_enable)(struct ftrace_event_call *);

include/trace/ftrace.h +1 -1

index 80e5f6c..a0de384 100644
@@ -568,7 +568,7 @@ static void ftrace_raw_event_##call(proto)				\
 		trace_nowake_buffer_unlock_commit(event, irq_flags, pc); \
 }									\
 									\
-static int ftrace_raw_reg_event_##call(void)				\
+static int ftrace_raw_reg_event_##call(void *ptr)			\

Shouldn't it have a __used attribute here, or something?

include/trace/ftrace.h static int ftrace_raw_reg_event_##call(void) \ · line 582

Quoted 11 lines

include/trace/ftrace.h +1 -1

 {									\
 	int ret;							\
 									\

@@ -579,7 +579,7 @@ static int ftrace_raw_reg_event_##call(void)				\
 	return ret;							\
 }									\
 									\
-static void ftrace_raw_unreg_event_##call(void)				\
+static void ftrace_raw_unreg_event_##call(void *ptr)			\

Same here.

Quoted 27 lines

include/trace/ftrace.h +0 -0

 {									\
 	unregister_trace_##call(ftrace_raw_event_##call);		\
 }									\

kernel/trace/trace_events.c +2 -2

index f95f847..1d289e2 100644
@@ -86,14 +86,14 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
 		if (call->enabled) {
 			call->enabled = 0;
 			tracing_stop_cmdline_record();
-			call->unregfunc();
+			call->unregfunc(call->data);
 		}
 		break;
 	case 1:
 		if (!call->enabled) {
 			call->enabled = 1;
 			tracing_start_cmdline_record();
-			call->regfunc();
+			call->regfunc(call->data);
 		}
 		break;
 	}

Signature · 1 line

1.6.2.5

Quoted 33 lines · Frederic Weisbecker

Quoted 32 lines · Jason Baron

add an optional * void pointer to 'ftrace_event_call' that is passed in for regfunc and unregfunc.

Signed-off-by Jason Baron <[email protected]>

---
 include/linux/ftrace_event.h |    5 +++--
 include/trace/ftrace.h       |    4 ++--
 kernel/trace/trace_events.c  |    4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

include/linux/ftrace_event.h +3 -2

index ac8c6f8..8544f12 100644
@@ -112,8 +112,8 @@ struct ftrace_event_call {
 	struct dentry		*dir;
 	struct trace_event	*event;
 	int			enabled;
-	int			(*regfunc)(void);
-	void			(*unregfunc)(void);
+	int			(*regfunc)(void *);
+	void			(*unregfunc)(void *);
 	int			id;
 	int			(*raw_init)(void);
 	int			(*show_format)(struct trace_seq *s);

@@ -122,6 +122,7 @@ struct ftrace_event_call {
 	int			filter_active;
 	struct event_filter	*filter;
 	void			*mod;
+	void			*data;
 

Quoted 15 lines

include/trace/ftrace.h static void ftrace_raw_event_##call(proto) \ · line 571

Quoted 12 lines

include/linux/ftrace_event.h +0 -0

 	atomic_t		profile_count;
 	int			(*profile_enable)(struct ftrace_event_call *);

include/trace/ftrace.h +1 -1

index 80e5f6c..a0de384 100644
@@ -568,7 +568,7 @@ static void ftrace_raw_event_##call(proto)				\
 		trace_nowake_buffer_unlock_commit(event, irq_flags, pc); \
 }									\
 									\
-static int ftrace_raw_reg_event_##call(void)				\
+static int ftrace_raw_reg_event_##call(void *ptr)			\

Shouldn't it have a __used attribute here, or something?

Do function parameters need that? There's lots of places where the parameter of a function is not used by a function itself.

-- Steve

Quoted 10 lines

Quoted 9 lines

include/trace/ftrace.h static void ftrace_raw_event_##call(proto) \ · line 571

Quoted 6 lines

include/trace/ftrace.h +1 -1

@@ -568,7 +568,7 @@ static void ftrace_raw_event_##call(proto)				\
 		trace_nowake_buffer_unlock_commit(event, irq_flags, pc); \
 }									\
 									\
-static int ftrace_raw_reg_event_##call(void)				\
+static int ftrace_raw_reg_event_##call(void *ptr)			\

Shouldn't it have a __used attribute here, or something?

Quoted 4 lines

Do function parameters need that? There's lots of places where the parameter of a function is not used by a function itself.

-- Steve

No actually, I thought gcc would warn, but it didn't :-)

Quoted 14 lines · Jason Baron

update FTRACE_SYSCALL_MAX to the current number of syscalls

Signed-off-by Jason Baron <[email protected]>

---
 arch/x86/include/asm/ftrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/include/asm/ftrace.h +0 -0

index bd2c651..7113654 100644
@@ -30,9 +30,9 @@
 

arch/x86/include/asm/ftrace.h line 36

Quoted 8 lines

arch/x86/include/asm/ftrace.h +2 -2

 /* FIXME: I don't want to stay hardcoded */
 #ifdef CONFIG_X86_64
-# define FTRACE_SYSCALL_MAX     296
+# define FTRACE_SYSCALL_MAX     299
 #else
-# define FTRACE_SYSCALL_MAX     333
+# define FTRACE_SYSCALL_MAX     337
 #endif

I don't remember why we had to use a hardcoded number. Is there no way to keep being sync with the current number of syscalls? We dwant to avoid patching the kernel each time we have a new syscall :-)

Quoted 15 lines · Jason Baron

Quoted 14 lines

update FTRACE_SYSCALL_MAX to the current number of syscalls

Signed-off-by Jason Baron <[email protected]>

---
 arch/x86/include/asm/ftrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/include/asm/ftrace.h +0 -0

index bd2c651..7113654 100644
@@ -30,9 +30,9 @@
 

Quoted 15 lines

arch/x86/include/asm/ftrace.h line 36

Quoted 8 lines

arch/x86/include/asm/ftrace.h +2 -2

 /* FIXME: I don't want to stay hardcoded */
 #ifdef CONFIG_X86_64
-# define FTRACE_SYSCALL_MAX     296
+# define FTRACE_SYSCALL_MAX     299
 #else
-# define FTRACE_SYSCALL_MAX     333
+# define FTRACE_SYSCALL_MAX     337
 #endif

I don't remember why we had to use a hardcoded number. Is there no way to keep being sync with the current number of syscalls? We dwant to avoid patching the kernel each time we have a new syscall :-)

I hope you can clarify what the meaning of this is supposed to be exactly. Is this number supposed to be the last usable syscall, or is it supposed to be the equivalent of NR_syscalls?

Presently on SH we have this as NR_syscalls - 1, while on s390 I see it is treated as NR_syscalls directly. s390 opencodes the NR_syscalls directly and so presently blows up in -next due to a missing FTRACE_SYSCALL_MAX definition:

	http://kisskb.ellerman.id.au/kisskb/buildresult/1120523/

I was in the process of fixing that up when I noticed this difference. x86 seems to also treat this as NR_syscalls - 1, but that looks to me like there is an off-by-1 in arch_init_ftrace_syscalls() causing the last syscall to be skipped?

Quoted 4 lines · Paul Mundt

I hope you can clarify what the meaning of this is supposed to be exactly. Is this number supposed to be the last usable syscall, or is it supposed to be the equivalent of NR_syscalls?

I am using as the equivalent of NR_syscalls.

Quoted 11 lines

Presently on SH we have this as NR_syscalls - 1, while on s390 I see it is treated as NR_syscalls directly. s390 opencodes the NR_syscalls directly and so presently blows up in -next due to a missing FTRACE_SYSCALL_MAX definition:

	http://kisskb.ellerman.id.au/kisskb/buildresult/1120523/

I was in the process of fixing that up when I noticed this difference. x86 seems to also treat this as NR_syscalls - 1, but that looks to me like there is an off-by-1 in arch_init_ftrace_syscalls() causing the last syscall to be skipped?

I don't see how its used as 'NR_syscalls - 1' on x86, arch_init_ftrace_syscalls() does:

        for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
                meta = find_syscall_meta(psys_syscall_table[i]);
                syscalls_metadata[i] = meta;
        }

So the last syscall should not be skipped.

Quoted 8 lines · Jason Baron

Quoted 4 lines · Paul Mundt

I hope you can clarify what the meaning of this is supposed to be exactly. Is this number supposed to be the last usable syscall, or is it supposed to be the equivalent of NR_syscalls?

I am using as the equivalent of NR_syscalls.

NR_syscalls has always been the total number of system calls, not the last one.

Quoted 15 lines

Quoted 11 lines

Presently on SH we have this as NR_syscalls - 1, while on s390 I see it is treated as NR_syscalls directly. s390 opencodes the NR_syscalls directly and so presently blows up in -next due to a missing FTRACE_SYSCALL_MAX definition:

	http://kisskb.ellerman.id.au/kisskb/buildresult/1120523/

I was in the process of fixing that up when I noticed this difference. x86 seems to also treat this as NR_syscalls - 1, but that looks to me like there is an off-by-1 in arch_init_ftrace_syscalls() causing the last syscall to be skipped?

I don't see how its used as 'NR_syscalls - 1' on x86, arch_init_ftrace_syscalls() does:

Quoted 7 lines

        for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
                meta = find_syscall_meta(psys_syscall_table[i]);
                syscalls_metadata[i] = meta;
        }

So the last syscall should not be skipped.

In today's -next:

#ifdef CONFIG_X86_64

# define FTRACE_SYSCALL_MAX     299
#else
# define FTRACE_SYSCALL_MAX     337
#endif

unistd_32.h:

#define __NR_reflinkat 337

unistd_64.h:

#define __NR_reflinkat 299

The first syscall starts at 0, but I don't see how this last syscall is handled. If there were a __NR_syscalls 300 and 338 respectively, that would seem to do the right thing. Or am I missing something?

Quoted 26 lines · Paul Mundt

Quoted 10 lines · Jason Baron

I don't see how its used as 'NR_syscalls - 1' on x86, arch_init_ftrace_syscalls() does:

        for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
                meta = find_syscall_meta(psys_syscall_table[i]);
                syscalls_metadata[i] = meta;
        }

So the last syscall should not be skipped.

In today's -next:

#ifdef CONFIG_X86_64

# define FTRACE_SYSCALL_MAX     299
#else
# define FTRACE_SYSCALL_MAX     337
#endif

unistd_32.h:

#define __NR_reflinkat 337

unistd_64.h:

Quoted 5 lines

#define __NR_reflinkat 299

The first syscall starts at 0, but I don't see how this last syscall is handled. If there were a __NR_syscalls 300 and 338 respectively, that would seem to do the right thing. Or am I missing something?

Yeah, I guess what we need here is NR_syscalls + 1.

Quoted 8 lines · Jason Baron

Quoted 4 lines · Paul Mundt

I hope you can clarify what the meaning of this is supposed to be exactly. Is this number supposed to be the last usable syscall, or is it supposed to be the equivalent of NR_syscalls?

I am using as the equivalent of NR_syscalls.

NR_syscalls has always been the total number of system calls, not the last one.

Quoted 15 lines

Quoted 11 lines

Presently on SH we have this as NR_syscalls - 1, while on s390 I see it is treated as NR_syscalls directly. s390 opencodes the NR_syscalls directly and so presently blows up in -next due to a missing FTRACE_SYSCALL_MAX definition:

	http://kisskb.ellerman.id.au/kisskb/buildresult/1120523/

I was in the process of fixing that up when I noticed this difference. x86 seems to also treat this as NR_syscalls - 1, but that looks to me like there is an off-by-1 in arch_init_ftrace_syscalls() causing the last syscall to be skipped?

I don't see how its used as 'NR_syscalls - 1' on x86, arch_init_ftrace_syscalls() does:

Quoted 7 lines

        for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
                meta = find_syscall_meta(psys_syscall_table[i]);
                syscalls_metadata[i] = meta;
        }

So the last syscall should not be skipped.

In today's -next:

#ifdef CONFIG_X86_64

# define FTRACE_SYSCALL_MAX     299
#else
# define FTRACE_SYSCALL_MAX     337
#endif

unistd_32.h:

#define __NR_reflinkat 337

unistd_64.h:

#define __NR_reflinkat 299

The first syscall starts at 0, but I don't see how this last syscall is handled. If there were a __NR_syscalls 300 and 338 respectively, that would seem to do the right thing. Or am I missing something?

Quoted 47 lines · Paul Mundt

Quoted 8 lines · Jason Baron

Quoted 4 lines · Paul Mundt

I hope you can clarify what the meaning of this is supposed to be exactly. Is this number supposed to be the last usable syscall, or is it supposed to be the equivalent of NR_syscalls?

I am using as the equivalent of NR_syscalls.

NR_syscalls has always been the total number of system calls, not the last one.

Quoted 15 lines

Quoted 11 lines

Presently on SH we have this as NR_syscalls - 1, while on s390 I see it is treated as NR_syscalls directly. s390 opencodes the NR_syscalls directly and so presently blows up in -next due to a missing FTRACE_SYSCALL_MAX definition:

	http://kisskb.ellerman.id.au/kisskb/buildresult/1120523/

I was in the process of fixing that up when I noticed this difference. x86 seems to also treat this as NR_syscalls - 1, but that looks to me like there is an off-by-1 in arch_init_ftrace_syscalls() causing the last syscall to be skipped?

I don't see how its used as 'NR_syscalls - 1' on x86, arch_init_ftrace_syscalls() does:

Quoted 7 lines

        for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
                meta = find_syscall_meta(psys_syscall_table[i]);
                syscalls_metadata[i] = meta;
        }

So the last syscall should not be skipped.

In today's -next:

#ifdef CONFIG_X86_64

# define FTRACE_SYSCALL_MAX     299
#else
# define FTRACE_SYSCALL_MAX     337
#endif

unistd_32.h:

#define __NR_reflinkat 337

Quoted 7 lines

unistd_64.h:

#define __NR_reflinkat 299

The first syscall starts at 0, but I don't see how this last syscall is handled. If there were a __NR_syscalls 300 and 338 respectively, that would seem to do the right thing. Or am I missing something?

No, you are right. When I changed the FTRACE_SYSCALL_MAX to 299, and 337, there was no reflinkat syscall in the tree. So, it was equivalent to NR_syscalls at that point in time. So that's where the confusion is.

Clearly, all the more reason to drop FTRACE_SYSCALL_MAX and change to NR_syscalls...

Quoted 56 lines · Jason Baron

Quoted 47 lines · Paul Mundt

Quoted 8 lines · Jason Baron

Quoted 4 lines · Paul Mundt

I hope you can clarify what the meaning of this is supposed to be exactly. Is this number supposed to be the last usable syscall, or is it supposed to be the equivalent of NR_syscalls?

I am using as the equivalent of NR_syscalls.

NR_syscalls has always been the total number of system calls, not the last one.

Quoted 15 lines

Quoted 11 lines

Presently on SH we have this as NR_syscalls - 1, while on s390 I see it is treated as NR_syscalls directly. s390 opencodes the NR_syscalls directly and so presently blows up in -next due to a missing FTRACE_SYSCALL_MAX definition:

	http://kisskb.ellerman.id.au/kisskb/buildresult/1120523/

I was in the process of fixing that up when I noticed this difference. x86 seems to also treat this as NR_syscalls - 1, but that looks to me like there is an off-by-1 in arch_init_ftrace_syscalls() causing the last syscall to be skipped?

I don't see how its used as 'NR_syscalls - 1' on x86, arch_init_ftrace_syscalls() does:

Quoted 7 lines

        for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
                meta = find_syscall_meta(psys_syscall_table[i]);
                syscalls_metadata[i] = meta;
        }

So the last syscall should not be skipped.

In today's -next:

#ifdef CONFIG_X86_64

# define FTRACE_SYSCALL_MAX     299
#else
# define FTRACE_SYSCALL_MAX     337
#endif

unistd_32.h:

#define __NR_reflinkat 337

Quoted 7 lines

unistd_64.h:

#define __NR_reflinkat 299

The first syscall starts at 0, but I don't see how this last syscall is handled. If there were a __NR_syscalls 300 and 338 respectively, that would seem to do the right thing. Or am I missing something?

Quoted 6 lines

No, you are right. When I changed the FTRACE_SYSCALL_MAX to 299, and 337, there was no reflinkat syscall in the tree. So, it was equivalent to NR_syscalls at that point in time. So that's where the confusion is.

Clearly, all the more reason to drop FTRACE_SYSCALL_MAX and change to NR_syscalls...

If FTRACE_SYSCALL_MAX is dropped then s390 will be fixed, and I'll take care of the sh update. If you want to hold off on adding NR_syscalls back to x86, then s390 will need a #define FTRACE_SYSCALL_MAX __NR_syscalls in arch/s390/include/asm/ftrace.h. Keeping FTRACE_SYSCALL_MAX around seems to be asking for trouble, though (although I don't know what the original rationale behind adding it was).

(quoted diff) +4 -0

+void ftrace_syscall_enter(struct pt_regs *regs, long id)
 {
 	struct syscall_trace_enter *entry;
 	struct syscall_metadata *sys_data;

@@ -150,6 +105,8 @@ void ftrace_syscall_enter(struct pt_regs *regs)
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_enter_syscalls))
+		return;
 
 	sys_data = syscall_nr_to_meta(syscall_nr);
 	if (!sys_data)
 
+void ftrace_syscall_exit(struct pt_regs *regs, long ret)
 {
 	struct syscall_trace_exit *entry;
 	struct syscall_metadata *sys_data;

Hi,

I looked at your recent syscall tracepoint patches and I have few more s390 arch updates.

This patch includes s390 arch updates for:
- tracing: Map syscall name to number (syscall_name_to_nr())
- tracing: Call arch_init_ftrace_syscalls at boot
- tracing: add support traceopint ids (set_syscall_{enter,exit}_id())

The patch already uses "NR_syscalls" instead of FTRACE_SYSCALL_MAX.

The patch is based on today's linux-next (20090825). Since few of your patches already include s390 changes, I would appreciate if you could add the patch to your patch set.

If you have any remarks, please let me know.

Signed-off-by Hendrik Brueckner <[email protected]>

---
 arch/s390/include/asm/ftrace.h |    4 ++++
 arch/s390/kernel/ftrace.c      |   36 +++++++++++++++++++++++++++---------
 2 files changed, 31 insertions(+), 9 deletions(-)

arch/s390/kernel/ftrace.c +11 -0

@@ -220,6 +220,29 @@ struct syscall_metadata *syscall_nr_to_m
 	return syscalls_metadata[nr];
 }
 
+int syscall_name_to_nr(char *name)
+{
+	int i;
+
+	if (!syscalls_metadata)
+		return -1;
+	for (i = 0; i < NR_syscalls; i++)
+		if (syscalls_metadata[i])
+			if (!strcmp(syscalls_metadata[i]->name, name))
+				return i;
+	return -1;

arch/s390/kernel/ftrace.c +2 -6

 static struct syscall_metadata *find_syscall_meta(unsigned long syscall)
 {
 	struct syscall_metadata *start;

@@ -237,24 +260,19 @@ static struct syscall_metadata *find_sys
 	return NULL;
 }
 
-void arch_init_ftrace_syscalls(void)
+static int __init arch_init_ftrace_syscalls(void)
 {
 	struct syscall_metadata *meta;
 	int i;
-	static atomic_t refs;
-
-	if (atomic_inc_return(&refs) != 1)
-		goto out;
 	syscalls_metadata = kzalloc(sizeof(*syscalls_metadata) * NR_syscalls,
 				    GFP_KERNEL);
 	if (!syscalls_metadata)
-		goto out;
+		return -ENOMEM;
 	for (i = 0; i < NR_syscalls; i++) {

Quoted 19 lines · Jason Baron

kernel/trace/trace_syscalls.c +4 -0

+void ftrace_syscall_enter(struct pt_regs *regs, long id)
 {
 	struct syscall_trace_enter *entry;
 	struct syscall_metadata *sys_data;

@@ -150,6 +105,8 @@ void ftrace_syscall_enter(struct pt_regs *regs)
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_enter_syscalls))
+		return;
 
 	sys_data = syscall_nr_to_meta(syscall_nr);
 	if (!sys_data)
 
+void ftrace_syscall_exit(struct pt_regs *regs, long ret)
 {
 	struct syscall_trace_exit *entry;
 	struct syscall_metadata *sys_data;

kernel/trace/trace_syscalls.c void ftrace_syscall_exit(struct pt_regs *regs) · line 139

Quoted 5 lines

kernel/trace/trace_syscalls.c +2 -0

@@ -178,6 +135,8 @@ void ftrace_syscall_exit(struct pt_regs *regs)
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_exit_syscalls))
+		return;

Most arch syscall_get_nr() implementations returns -1 if the syscall number is not valid. Accessing the bit field without a check might result in a kernel oops (at least I saw it on s390 for ftrace selftest).

Before this change, this problem did not occur, because the invalid syscall number (-1) caused syscall_nr_to_meta() to return NULL.

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

To avoid these scenarios, I suggest to check the syscall_nr.

For instance, the ftrace selftest fails for s390 (with config option CONFIG_FTRACE_SYSCALLS set) and produces the following kernel oops.

Unable to handle kernel pointer dereference at virtual kernel address 2000000000

Oops: 0038 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 Not tainted 2.6.31-rc6-next-20090819-dirty #18
Process kthreadd (pid: 818, task: 000000003ea207e8, ksp: 000000003e813eb8)
Krnl PSW : 0704100180000000 00000000000ea54c (ftrace_syscall_exit+0x58/0xdc)
           R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:0 CC:1 PM:0 EA:3
Krnl GPRS: 0000000000000000 00000000000e0000 ffffffffffffffff 20000000008c2650
           0000000000000007 0000000000000000 0000000000000000 0000000000000000
           0000000000000000 0000000000000000 ffffffffffffffff 000000003e813d78
           000000003e813f58 0000000000505ba8 000000003e813e18 000000003e813d78
Krnl Code: 00000000000ea540: e330d0000008       ag      %r3,0(%r13)
           00000000000ea546: a7480007           lhi     %r4,7
           00000000000ea54a: 1442               nr      %r4,%r2
          >00000000000ea54c: e31030000090       llgc    %r1,0(%r3)
           00000000000ea552: 5410d008           n       %r1,8(%r13)
           00000000000ea556: 8a104000           sra     %r1,0(%r4)
           00000000000ea55a: 5410d00c           n       %r1,12(%r13)
           00000000000ea55e: 1211               ltr     %r1,%r1
Call Trace:
([<0000000000000000>] 0x0)
 [<000000000001fa22>] do_syscall_trace_exit+0x132/0x18c
 [<000000000002d0c4>] sysc_return+0x0/0x8
 [<000000000001c738>] kernel_thread_starter+0x0/0xc

Last Breaking-Event-Address:
[<00000000000ea51e>] ftrace_syscall_exit+0x2a/0xdc

Signed-off-by Hendrik Brueckner <[email protected]>

arch/s390/kernel/ftrace.c static struct syscall_metadata *find_sys · line 274

Quoted 3 lines · Hendrik Brueckner

arch/s390/kernel/ftrace.c +0 -0

 		meta = find_syscall_meta((unsigned long)sys_call_table[i]);
 		syscalls_metadata[i] = meta;
 	}

We can even probably move most of this code to the core, expect the tiny parts that rely on the arch syscall table.

BTW, perhaps a silly question: would it be hard to have a generic syscall table common to every archs?

Quoted 11 lines · Hendrik Brueckner

This patch includes s390 arch updates for:
- tracing: Map syscall name to number (syscall_name_to_nr())
- tracing: Call arch_init_ftrace_syscalls at boot
- tracing: add support traceopint ids (set_syscall_{enter,exit}_id())

The patch already uses "NR_syscalls" instead of FTRACE_SYSCALL_MAX.

The patch is based on today's linux-next (20090825). Since few of your patches already include s390 changes, I would appreciate if you could add the patch to your patch set.

Quoted 3 lines

If you have any remarks, please let me know.

Signed-off-by Hendrik Brueckner <[email protected]>

Looks good at a first glance.

Quoted 16 lines

arch/s390/kernel/ftrace.c +16 -0

+int syscall_name_to_nr(char *name)
+{
+	int i;
+
+	if (!syscalls_metadata)
+		return -1;
+	for (i = 0; i < NR_syscalls; i++)
+		if (syscalls_metadata[i])
+			if (!strcmp(syscalls_metadata[i]->name, name))
+				return i;
+	return -1;
+}
+void set_syscall_enter_id(int num, int id)
+{
+	syscalls_metadata[num]->enter_id = id;
+}

arch/s390/kernel/ftrace.c struct syscall_metadata *syscall_nr_to_m · line 244

Quoted 5 lines

arch/s390/kernel/ftrace.c +5 -0

+
+void set_syscall_exit_id(int num, int id)
+{
+	syscalls_metadata[num]->exit_id = id;
+}

The three helpers above seem very common between archs, I guess we can move them to the core: kernel/trace/trace_syscalls.c

Quoted 6 lines · Hendrik Brueckner

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

Quoted 9 lines

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

I wonder if there is any way to identify such situation...?

Quoted 19 lines · Frederic Weisbecker

Quoted 6 lines · Hendrik Brueckner

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

Quoted 9 lines

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

Quoted 1 line

I wonder if there is any way to identify such situation...?

For the second case, it might be an option to avoid setting the TIF_SYSCALL_FTRACE flag for kernel threads.

Kernel threads have task_struct->mm set to NULL.
(Thanks to Heiko for that hint ;-)

The idea is then to check the mm field in syscall_regfunc() and set the flag accordingly.

However, I think the patch is an optional add-on becase checking the syscall number is still required for case 1).

---
 kernel/tracepoint.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

kernel/tracepoint.c +0 -0

 		} while_each_thread(g, t);
 		read_unlock_irqrestore(&tasklist_lock, flags);
 	}

Quoted 37 lines · * Hendrik Brueckner ([email protected]) wrote:

Quoted 19 lines · Frederic Weisbecker

Quoted 6 lines · Hendrik Brueckner

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

Quoted 9 lines

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

Quoted 1 line

I wonder if there is any way to identify such situation...?

For the second case, it might be an option to avoid setting the TIF_SYSCALL_FTRACE flag for kernel threads.

Kernel threads have task_struct->mm set to NULL.
(Thanks to Heiko for that hint ;-)

The idea is then to check the mm field in syscall_regfunc() and set the flag accordingly.

However, I think the patch is an optional add-on becase checking the syscall number is still required for case 1).

---
 kernel/tracepoint.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

kernel/tracepoint.c void syscall_regfunc(void) · line 598

Quoted 10 lines

kernel/tracepoint.c +3 -1

@@ -593,7 +593,9 @@ void syscall_regfunc(void)
 	if (!sys_tracepoint_refcount) {
 		read_lock_irqsave(&tasklist_lock, flags);
 		do_each_thread(g, t) {
-			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
+			/* Skip kernel threads. */
+			if (t->mm)
+				set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);

Uh ? kernel threads can invoke a system call. There are rare places where kernel code actually invoke system calls. I don't see why we should not deal with them.

Moreover, the problem you face is more general: if we set the TIF_SYSCALL_FTRACE flag of a standard thread right in the middle of its system call, x86_64 will cause the syscall exit to execute by re-reading the thread flags and run a syscall trace exit.

We could simply initialize the "saved system calls id" number to something like -1, so that if we happen to return from a syscall that did not get its id recorded at syscall entry, we know it because it's not initialized.

We would need to carefully put back the -1 value after clearing the thread flag when we stop tracing too (while still holding a mutex).

Mathieu

Quoted 4 lines

kernel/tracepoint.c +0 -0

 		} while_each_thread(g, t);
 		read_unlock_irqrestore(&tasklist_lock, flags);
 	}

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Uh ? kernel threads can invoke a system call. There are rare places where kernel code actually invoke system calls. I don't see why we should not deal with them.

Moreover, the problem you face is more general: if we set the TIF_SYSCALL_FTRACE flag of a standard thread right in the middle of its system call, x86_64 will cause the syscall exit to execute by re-reading the thread flags and run a syscall trace exit.

We could simply initialize the "saved system calls id" number to something like -1, so that if we happen to return from a syscall that did not get its id recorded at syscall entry, we know it because it's not initialized.

We would need to carefully put back the -1 value after clearing the thread flag when we stop tracing too (while still holding a mutex).

Mathieu

Quoted 38 lines · Mathieu Desnoyers

Quoted 37 lines · * Hendrik Brueckner ([email protected]) wrote:

Quoted 19 lines · Frederic Weisbecker

Quoted 6 lines · Hendrik Brueckner

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

Quoted 9 lines

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

Quoted 1 line

I wonder if there is any way to identify such situation...?

For the second case, it might be an option to avoid setting the TIF_SYSCALL_FTRACE flag for kernel threads.

Kernel threads have task_struct->mm set to NULL.
(Thanks to Heiko for that hint ;-)

The idea is then to check the mm field in syscall_regfunc() and set the flag accordingly.

However, I think the patch is an optional add-on becase checking the syscall number is still required for case 1).

---
 kernel/tracepoint.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Quoted 14 lines

kernel/tracepoint.c void syscall_regfunc(void) · line 598

Quoted 10 lines

kernel/tracepoint.c +3 -1

@@ -593,7 +593,9 @@ void syscall_regfunc(void)
 	if (!sys_tracepoint_refcount) {
 		read_lock_irqsave(&tasklist_lock, flags);
 		do_each_thread(g, t) {
-			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
+			/* Skip kernel threads. */
+			if (t->mm)
+				set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);

Uh ? kernel threads can invoke a system call. There are rare places where kernel code actually invoke system calls. I don't see why we should not deal with them.

Yeah they do, but they don't use the sysenter path, they call the syscall helpers directly, such as do_fork() or things like that.

The syscall tracepoints are set in the sysenter/sysexit path, then it's no use to trace the kernel threads, it doesn't have any effect, except random results in case of fork() calls, because we take the ret_from_fork() path that also ends up to trace_sys_exit() if the TIF_SYSCALL_TRACEPOINT thing is set, leading to such asymetric tracing.

Kernel threads use syscalls toward wrappers such as create_thread(). So instead, statically defined tracepoints in create_thread() and such other syscall wrappers for kernel threads seem more valuable, hmm?

Quoted 4 lines

Moreover, the problem you face is more general: if we set the TIF_SYSCALL_FTRACE flag of a standard thread right in the middle of its system call, x86_64 will cause the syscall exit to execute by re-reading the thread flags and run a syscall trace exit.

Well, I don't think that's the problem. The issue here, if I understand correctly, is that kernel threads don't take the sysenter path, then never hit the trace_sys_enter() call. And usually they won't ever hit any trace_sys_exit() calls except in the fork() case, because we take the ret_from_fork() path, which lead to syscall exit tracing due to the TIF flags set.

At this stage, the syscall number is supposed to be stored in orig_eax, but because the kernel thread hasn't called fork() through a syscall and has called do_fork() directly, the regs values have nothing that look like syscall parameters.

I guess we don't need to take the sys_enter tracing path to have a sane orig_eax in the sys_exit tracing path (for non kernel threads). Though I'm not sure about that, I should check to be sure.

Quoted 18 lines

We could simply initialize the "saved system calls id" number to something like -1, so that if we happen to return from a syscall that did not get its id recorded at syscall entry, we know it because it's not initialized.

We would need to carefully put back the -1 value after clearing the thread flag when we stop tracing too (while still holding a mutex).

Mathieu

Quoted 4 lines

kernel/tracepoint.c +0 -0

 		} while_each_thread(g, t);
 		read_unlock_irqrestore(&tasklist_lock, flags);
 	}

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Quoted 18 lines · Mathieu Desnoyers

Uh ? kernel threads can invoke a system call. There are rare places where kernel code actually invoke system calls. I don't see why we should not deal with them.

Moreover, the problem you face is more general: if we set the TIF_SYSCALL_FTRACE flag of a standard thread right in the middle of its system call, x86_64 will cause the syscall exit to execute by re-reading the thread flags and run a syscall trace exit.

We could simply initialize the "saved system calls id" number to something like -1, so that if we happen to return from a syscall that did not get its id recorded at syscall entry, we know it because it's not initialized.

We would need to carefully put back the -1 value after clearing the thread flag when we stop tracing too (while still holding a mutex).

Quoted 2 lines

Mathieu

why can't we have a syscall exit that is unmatched? we calculate the exit syscall number for the the pt_regs structure at exit, so we don't need to match it up with an entry to know which syscall it is.

thanks,

-Jason

Quoted 84 lines · Frederic Weisbecker

Quoted 38 lines · Mathieu Desnoyers

Quoted 37 lines · * Hendrik Brueckner ([email protected]) wrote:

Quoted 19 lines · Frederic Weisbecker

Quoted 6 lines · Hendrik Brueckner

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

Quoted 9 lines

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

Quoted 1 line

I wonder if there is any way to identify such situation...?

For the second case, it might be an option to avoid setting the TIF_SYSCALL_FTRACE flag for kernel threads.

Kernel threads have task_struct->mm set to NULL.
(Thanks to Heiko for that hint ;-)

The idea is then to check the mm field in syscall_regfunc() and set the flag accordingly.

However, I think the patch is an optional add-on becase checking the syscall number is still required for case 1).

---
 kernel/tracepoint.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Quoted 14 lines

kernel/tracepoint.c void syscall_regfunc(void) · line 598

Quoted 10 lines

kernel/tracepoint.c +3 -1

@@ -593,7 +593,9 @@ void syscall_regfunc(void)
 	if (!sys_tracepoint_refcount) {
 		read_lock_irqsave(&tasklist_lock, flags);
 		do_each_thread(g, t) {
-			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
+			/* Skip kernel threads. */
+			if (t->mm)
+				set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);

Uh ? kernel threads can invoke a system call. There are rare places where kernel code actually invoke system calls. I don't see why we should not deal with them.

Yeah they do, but they don't use the sysenter path, they call the syscall helpers directly, such as do_fork() or things like that.

The syscall tracepoints are set in the sysenter/sysexit path, then it's no use to trace the kernel threads, it doesn't have any effect, except random results in case of fork() calls, because we take the ret_from_fork() path that also ends up to trace_sys_exit() if the TIF_SYSCALL_TRACEPOINT thing is set, leading to such asymetric tracing.

Kernel threads use syscalls toward wrappers such as create_thread(). So instead, statically defined tracepoints in create_thread() and such other syscall wrappers for kernel threads seem more valuable, hmm?

Quoted 4 lines

Moreover, the problem you face is more general: if we set the TIF_SYSCALL_FTRACE flag of a standard thread right in the middle of its system call, x86_64 will cause the syscall exit to execute by re-reading the thread flags and run a syscall trace exit.

Well, I don't think that's the problem. The issue here, if I understand correctly, is that kernel threads don't take the sysenter path, then never hit the trace_sys_enter() call. And usually they won't ever hit any trace_sys_exit() calls except in the fork() case, because we take the ret_from_fork() path, which lead to syscall exit tracing due to the TIF flags set.

Quoted 4 lines

At this stage, the syscall number is supposed to be stored in orig_eax, but because the kernel thread hasn't called fork() through a syscall and has called do_fork() directly, the regs values have nothing that look like syscall parameters.

I mean, I don't know how look like orig_eax at this stage.

Looking at arch/x86/kernel/process_32.c:copy_thread()

childregs = task_pt_regs(p);
*childregs = *regs;
childregs->ax = 0;
childregs->sp = sp;

p->thread.sp = (unsigned long) childregs;
p->thread.sp0 = (unsigned long) (childregs+1);

p->thread.ip = (unsigned long) ret_from_fork;

sp will be the struct pt_regs * passed to syscall_trace_leave() later.

ax has the result of the fork syscall -> 0 for the child. What about orig_eax which has the syscall nr? It depends on the pareent and I don't know what it has at this stage.

I haven't seen crashes in x86 with kernel threads tracing, may be because orig_eax is set to a valid syscall nr (may be even fork nr).

Perhaps it's not the case in s390 ?

Anyway, tracing kernel threads syscalls only gives us the fork return, so it's something me may want to drop and trace higher level kernel thread syscall wrappers instead.

Moreover every kernel threads is created through a kthreadd fork if I'm not wrong, then it wouldn't be an accurate thing for us to trace the fork calls in kernel thread. Tracing higher level kernel thread managment sounds more interesting, we would then know who really created the thread, etc...

Quoted 25 lines

I guess we don't need to take the sys_enter tracing path to have a sane orig_eax in the sys_exit tracing path (for non kernel threads). Though I'm not sure about that, I should check to be sure.

Quoted 18 lines

We could simply initialize the "saved system calls id" number to something like -1, so that if we happen to return from a syscall that did not get its id recorded at syscall entry, we know it because it's not initialized.

We would need to carefully put back the -1 value after clearing the thread flag when we stop tracing too (while still holding a mutex).

Mathieu

Quoted 4 lines

kernel/tracepoint.c +0 -0

 		} while_each_thread(g, t);
 		read_unlock_irqrestore(&tasklist_lock, flags);
 	}

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

(Well, I do not have time currently to look into the gory details (sorry), but let's try to take a step back from the problem.)

The design proposal for this kthread behavior wrt syscalls is based on a very specific and current kernel behavior, that may happen to change and that I have actually seen proven incorrect. For instance, some proprietary Linux driver does very odd things with system calls within kernel threads, like invoking them with int 0x80.

Yes, this is odd, but do we really want to tie the tracer that much to the actual OS implementation specificities ?

That sounds like a recipe for endless breakages and missing bits of instrumentation.

So my advice would be: if we want to trace the syscall entry/exit paths, let's trace them for the _whole_ system, and find ways to make it work for corner-cases rather than finding clever ways to diminish instrumentation coverage.

Given the ret from fork example happens to be the first event fired after the thread is created, we should be able to deal with this problem by initializing the thread structure used by syscall exit tracing to an initial "ret from fork" value.

Mathieu

Quoted 11 lines · Mathieu Desnoyers

(Well, I do not have time currently to look into the gory details (sorry), but let's try to take a step back from the problem.)

The design proposal for this kthread behavior wrt syscalls is based on a very specific and current kernel behavior, that may happen to change and that I have actually seen proven incorrect. For instance, some proprietary Linux driver does very odd things with system calls within kernel threads, like invoking them with int 0x80.

Yes, this is odd, but do we really want to tie the tracer that much to the actual OS implementation specificities ?

I really can't see the point in doing this. I don't expect the kernel behaviour to change soon and have explicit syscalls interrupts done from it. It's not about a current kernel implementation fashion, it's about kernel design sanity that is not likely to go backward.

Is it worth it to trace kernel threads, maintain their tracing specificities (such as workarounds with ret_from_fork that implies) just because we want to support tracing on some silly proprietary drivers?

Quoted 8 lines

That sounds like a recipe for endless breakages and missing bits of instrumentation.

So my advice would be: if we want to trace the syscall entry/exit paths, let's trace them for the _whole_ system, and find ways to make it work for corner-cases rather than finding clever ways to diminish instrumentation coverage.

If developers of out of tree drivers want to implement buggy things that would never be accepted after a minimal review here, and then instrument their bugs, then I would suggest them to implement their own ad hoc instrumentation, really :-/

What's the point in supporting out of tree bugs?

Well, the only advantage of doing this would be to support reverse engineering in tiny and rare corner cases. Not that worth the effort.

Quoted 6 lines

Given the ret from fork example happens to be the first event fired after the thread is created, we should be able to deal with this problem by initializing the thread structure used by syscall exit tracing to an initial "ret from fork" value.

Mathieu

It means we have to support and check this corner case in every archs that support syscall tracing, deal with crashes because we omitted it, etc...

For all the things I've explained above I don't think it's worth the effort.

But it's just my opinion...

Quoted 7 lines · Frederic Weisbecker

arch/s390/kernel/ftrace.c static struct syscall_metadata *find_sys · line 274

Quoted 3 lines · Hendrik Brueckner

arch/s390/kernel/ftrace.c +0 -0

 		meta = find_syscall_meta((unsigned long)sys_call_table[i]);
 		syscalls_metadata[i] = meta;
 	}

We can even probably move most of this code to the core, expect the tiny parts that rely on the arch syscall table.

Quoted 2 lines

BTW, perhaps a silly question: would it be hard to have a generic syscall table common to every archs?

That would cause a lot of churn. Every architecture initializes the syscall table (two tables if CONFIG_COMPAT is enabled) differently. s390 also only uses 32 bit pointers in the system call table for 64 bit kernels, since we know that the functions are within the first 4GB. I don't think its worth the effort.

Most arch syscall_get_nr() implementations returns -1 if the syscall number is not valid. Accessing the bit field without a check might result in a kernel oops (at least I saw it on s390 for ftrace selftest).

Before this change, this problem did not occur, because the invalid syscall number (-1) caused syscall_nr_to_meta() to return NULL.

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

To avoid these scenarios, I suggest to check the syscall_nr.

For instance, the ftrace selftest fails for s390 (with config option CONFIG_FTRACE_SYSCALLS set) and produces the following kernel oops.

Unable to handle kernel pointer dereference at virtual kernel address 2000000000

Oops: 0038 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 Not tainted 2.6.31-rc6-next-20090819-dirty #18
Process kthreadd (pid: 818, task: 000000003ea207e8, ksp: 000000003e813eb8)
Krnl PSW : 0704100180000000 00000000000ea54c (ftrace_syscall_exit+0x58/0xdc)
           R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:0 CC:1 PM:0 EA:3
Krnl GPRS: 0000000000000000 00000000000e0000 ffffffffffffffff 20000000008c2650
           0000000000000007 0000000000000000 0000000000000000 0000000000000000
           0000000000000000 0000000000000000 ffffffffffffffff 000000003e813d78
           000000003e813f58 0000000000505ba8 000000003e813e18 000000003e813d78
Krnl Code: 00000000000ea540: e330d0000008       ag      %r3,0(%r13)
           00000000000ea546: a7480007           lhi     %r4,7
           00000000000ea54a: 1442               nr      %r4,%r2
          >00000000000ea54c: e31030000090       llgc    %r1,0(%r3)
           00000000000ea552: 5410d008           n       %r1,8(%r13)
           00000000000ea556: 8a104000           sra     %r1,0(%r4)
           00000000000ea55a: 5410d00c           n       %r1,12(%r13)
           00000000000ea55e: 1211               ltr     %r1,%r1
Call Trace:
([<0000000000000000>] 0x0)
 [<000000000001fa22>] do_syscall_trace_exit+0x132/0x18c
 [<000000000002d0c4>] sysc_return+0x0/0x8
 [<000000000001c738>] kernel_thread_starter+0x0/0xc

Last Breaking-Event-Address:
[<00000000000ea51e>] ftrace_syscall_exit+0x2a/0xdc

Signed-off-by Hendrik Brueckner <[email protected]>

---
 kernel/trace/trace_syscalls.c |    4 ++++
 1 file changed, 4 insertions(+)

kernel/trace/trace_syscalls.c +4 -0

@@ -224,6 +224,8 @@ void ftrace_syscall_enter(struct pt_regs
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (syscall_nr < 0)
+		return;
 	if (!test_bit(syscall_nr, enabled_enter_syscalls))
 		return;
 

@@ -254,6 +256,8 @@ void ftrace_syscall_exit(struct pt_regs 
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (syscall_nr < 0)
+		return;
 	if (!test_bit(syscall_nr, enabled_exit_syscalls))
 		return;
 

Quoted 123 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 84 lines · Frederic Weisbecker

Quoted 38 lines · Mathieu Desnoyers

Quoted 37 lines · * Hendrik Brueckner ([email protected]) wrote:

Quoted 19 lines · Frederic Weisbecker

Quoted 6 lines · Hendrik Brueckner

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

Quoted 9 lines

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

Quoted 1 line

I wonder if there is any way to identify such situation...?

For the second case, it might be an option to avoid setting the TIF_SYSCALL_FTRACE flag for kernel threads.

Kernel threads have task_struct->mm set to NULL.
(Thanks to Heiko for that hint ;-)

The idea is then to check the mm field in syscall_regfunc() and set the flag accordingly.

However, I think the patch is an optional add-on becase checking the syscall number is still required for case 1).

---
 kernel/tracepoint.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Quoted 14 lines

kernel/tracepoint.c void syscall_regfunc(void) · line 598

Quoted 10 lines

kernel/tracepoint.c +3 -1

@@ -593,7 +593,9 @@ void syscall_regfunc(void)
 	if (!sys_tracepoint_refcount) {
 		read_lock_irqsave(&tasklist_lock, flags);
 		do_each_thread(g, t) {
-			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
+			/* Skip kernel threads. */
+			if (t->mm)
+				set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);

Uh ? kernel threads can invoke a system call. There are rare places where kernel code actually invoke system calls. I don't see why we should not deal with them.

Yeah they do, but they don't use the sysenter path, they call the syscall helpers directly, such as do_fork() or things like that.

The syscall tracepoints are set in the sysenter/sysexit path, then it's no use to trace the kernel threads, it doesn't have any effect, except random results in case of fork() calls, because we take the ret_from_fork() path that also ends up to trace_sys_exit() if the TIF_SYSCALL_TRACEPOINT thing is set, leading to such asymetric tracing.

Kernel threads use syscalls toward wrappers such as create_thread(). So instead, statically defined tracepoints in create_thread() and such other syscall wrappers for kernel threads seem more valuable, hmm?

Quoted 4 lines

Moreover, the problem you face is more general: if we set the TIF_SYSCALL_FTRACE flag of a standard thread right in the middle of its system call, x86_64 will cause the syscall exit to execute by re-reading the thread flags and run a syscall trace exit.

Well, I don't think that's the problem. The issue here, if I understand correctly, is that kernel threads don't take the sysenter path, then never hit the trace_sys_enter() call. And usually they won't ever hit any trace_sys_exit() calls except in the fork() case, because we take the ret_from_fork() path, which lead to syscall exit tracing due to the TIF flags set.

Quoted 4 lines

At this stage, the syscall number is supposed to be stored in orig_eax, but because the kernel thread hasn't called fork() through a syscall and has called do_fork() directly, the regs values have nothing that look like syscall parameters.

I mean, I don't know how look like orig_eax at this stage.

Looking at arch/x86/kernel/process_32.c:copy_thread()

childregs = task_pt_regs(p);
*childregs = *regs;
childregs->ax = 0;
childregs->sp = sp;

p->thread.sp = (unsigned long) childregs;
p->thread.sp0 = (unsigned long) (childregs+1);

p->thread.ip = (unsigned long) ret_from_fork;

sp will be the struct pt_regs * passed to syscall_trace_leave() later.

ax has the result of the fork syscall -> 0 for the child. What about orig_eax which has the syscall nr? It depends on the pareent and I don't know what it has at this stage.

I haven't seen crashes in x86 with kernel threads tracing, may be because orig_eax is set to a valid syscall nr (may be even fork nr).

Perhaps it's not the case in s390 ?

Anyway, tracing kernel threads syscalls only gives us the fork return, so it's something me may want to drop and trace higher level kernel thread syscall wrappers instead.

Quoted 6 lines

Moreover every kernel threads is created through a kthreadd fork if I'm not wrong, then it wouldn't be an accurate thing for us to trace the fork calls in kernel thread. Tracing higher level kernel thread managment sounds more interesting, we would then know who really created the thread, etc...

(Well, I do not have time currently to look into the gory details (sorry), but let's try to take a step back from the problem.)

The design proposal for this kthread behavior wrt syscalls is based on a very specific and current kernel behavior, that may happen to change and that I have actually seen proven incorrect. For instance, some proprietary Linux driver does very odd things with system calls within kernel threads, like invoking them with int 0x80.

Yes, this is odd, but do we really want to tie the tracer that much to the actual OS implementation specificities ?

That sounds like a recipe for endless breakages and missing bits of instrumentation.

So my advice would be: if we want to trace the syscall entry/exit paths, let's trace them for the _whole_ system, and find ways to make it work for corner-cases rather than finding clever ways to diminish instrumentation coverage.

Given the ret from fork example happens to be the first event fired after the thread is created, we should be able to deal with this problem by initializing the thread structure used by syscall exit tracing to an initial "ret from fork" value.

Quoted 56 lines · Hendrik Brueckner

Most arch syscall_get_nr() implementations returns -1 if the syscall number is not valid. Accessing the bit field without a check might result in a kernel oops (at least I saw it on s390 for ftrace selftest).

Before this change, this problem did not occur, because the invalid syscall number (-1) caused syscall_nr_to_meta() to return NULL.

There are at least two scenarios where syscall_get_nr() can return -1:

1. For example, ptrace stores an invalid syscall number, and thus, tracing code resets it.
(see do_syscall_trace_enter in arch/s390/kernel/ptrace.c)

2. The syscall_regfunc() (kernel/tracepoint.c) sets the TIF_SYSCALL_FTRACE (now: TIF_SYSCALL_TRACEPOINT) flag for all threads which includes kernel threads.
However, the ftrace selftest triggers a kernel oops when testing syscall trace points:
- The kernel thread is started as ususal (do_fork()),
- tracing code sets TIF_SYSCALL_FTRACE,
- the ret_from_fork() function is triggered and starts ftrace_syscall_exit() with an invalid syscall number.

To avoid these scenarios, I suggest to check the syscall_nr.

For instance, the ftrace selftest fails for s390 (with config option CONFIG_FTRACE_SYSCALLS set) and produces the following kernel oops.

Unable to handle kernel pointer dereference at virtual kernel address 2000000000

Oops: 0038 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 Not tainted 2.6.31-rc6-next-20090819-dirty #18
Process kthreadd (pid: 818, task: 000000003ea207e8, ksp: 000000003e813eb8)
Krnl PSW : 0704100180000000 00000000000ea54c (ftrace_syscall_exit+0x58/0xdc)
           R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:0 CC:1 PM:0 EA:3
Krnl GPRS: 0000000000000000 00000000000e0000 ffffffffffffffff 20000000008c2650
           0000000000000007 0000000000000000 0000000000000000 0000000000000000
           0000000000000000 0000000000000000 ffffffffffffffff 000000003e813d78
           000000003e813f58 0000000000505ba8 000000003e813e18 000000003e813d78
Krnl Code: 00000000000ea540: e330d0000008       ag      %r3,0(%r13)
           00000000000ea546: a7480007           lhi     %r4,7
           00000000000ea54a: 1442               nr      %r4,%r2
          >00000000000ea54c: e31030000090       llgc    %r1,0(%r3)
           00000000000ea552: 5410d008           n       %r1,8(%r13)
           00000000000ea556: 8a104000           sra     %r1,0(%r4)
           00000000000ea55a: 5410d00c           n       %r1,12(%r13)
           00000000000ea55e: 1211               ltr     %r1,%r1
Call Trace:
([<0000000000000000>] 0x0)
 [<000000000001fa22>] do_syscall_trace_exit+0x132/0x18c
 [<000000000002d0c4>] sysc_return+0x0/0x8
 [<000000000001c738>] kernel_thread_starter+0x0/0xc

Last Breaking-Event-Address:
[<00000000000ea51e>] ftrace_syscall_exit+0x2a/0xdc

Quoted 1 line

Signed-off-by Hendrik Brueckner <[email protected]>

I'm queueing this one for .32

Thanks.

Quoted 53 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 11 lines · Mathieu Desnoyers

(Well, I do not have time currently to look into the gory details (sorry), but let's try to take a step back from the problem.)

The design proposal for this kthread behavior wrt syscalls is based on a very specific and current kernel behavior, that may happen to change and that I have actually seen proven incorrect. For instance, some proprietary Linux driver does very odd things with system calls within kernel threads, like invoking them with int 0x80.

Yes, this is odd, but do we really want to tie the tracer that much to the actual OS implementation specificities ?

I really can't see the point in doing this. I don't expect the kernel behaviour to change soon and have explicit syscalls interrupts done from it. It's not about a current kernel implementation fashion, it's about kernel design sanity that is not likely to go backward.

Is it worth it to trace kernel threads, maintain their tracing specificities (such as workarounds with ret_from_fork that implies) just because we want to support tracing on some silly proprietary drivers?

Quoted 8 lines

That sounds like a recipe for endless breakages and missing bits of instrumentation.

So my advice would be: if we want to trace the syscall entry/exit paths, let's trace them for the _whole_ system, and find ways to make it work for corner-cases rather than finding clever ways to diminish instrumentation coverage.

If developers of out of tree drivers want to implement buggy things that would never be accepted after a minimal review here, and then instrument their bugs, then I would suggest them to implement their own ad hoc instrumentation, really :-/

What's the point in supporting out of tree bugs?

Well, the only advantage of doing this would be to support reverse engineering in tiny and rare corner cases. Not that worth the effort.

Quoted 6 lines

Given the ret from fork example happens to be the first event fired after the thread is created, we should be able to deal with this problem by initializing the thread structure used by syscall exit tracing to an initial "ret from fork" value.

Mathieu

Quoted 7 lines

It means we have to support and check this corner case in every archs that support syscall tracing, deal with crashes because we omitted it, etc...

For all the things I've explained above I don't think it's worth the effort.

But it's just my opinion...

Then we might want to explicitly require that calls to sys_*() system calls made from within the kernel pass through another instrumentation mechanism. IMHO, that would make sense. It would cover both system calls made from kernel threads and system calls made from within a system call or trap.

Mathieu

Quoted 62 lines · Mathieu Desnoyers

Quoted 53 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 11 lines · Mathieu Desnoyers

(Well, I do not have time currently to look into the gory details (sorry), but let's try to take a step back from the problem.)

The design proposal for this kthread behavior wrt syscalls is based on a very specific and current kernel behavior, that may happen to change and that I have actually seen proven incorrect. For instance, some proprietary Linux driver does very odd things with system calls within kernel threads, like invoking them with int 0x80.

Yes, this is odd, but do we really want to tie the tracer that much to the actual OS implementation specificities ?

I really can't see the point in doing this. I don't expect the kernel behaviour to change soon and have explicit syscalls interrupts done from it. It's not about a current kernel implementation fashion, it's about kernel design sanity that is not likely to go backward.

Is it worth it to trace kernel threads, maintain their tracing specificities (such as workarounds with ret_from_fork that implies) just because we want to support tracing on some silly proprietary drivers?

Quoted 8 lines

That sounds like a recipe for endless breakages and missing bits of instrumentation.

So my advice would be: if we want to trace the syscall entry/exit paths, let's trace them for the _whole_ system, and find ways to make it work for corner-cases rather than finding clever ways to diminish instrumentation coverage.

If developers of out of tree drivers want to implement buggy things that would never be accepted after a minimal review here, and then instrument their bugs, then I would suggest them to implement their own ad hoc instrumentation, really :-/

What's the point in supporting out of tree bugs?

Well, the only advantage of doing this would be to support reverse engineering in tiny and rare corner cases. Not that worth the effort.

Quoted 6 lines

Given the ret from fork example happens to be the first event fired after the thread is created, we should be able to deal with this problem by initializing the thread structure used by syscall exit tracing to an initial "ret from fork" value.

Mathieu

Quoted 7 lines

It means we have to support and check this corner case in every archs that support syscall tracing, deal with crashes because we omitted it, etc...

For all the things I've explained above I don't think it's worth the effort.

But it's just my opinion...

Quoted 7 lines

Then we might want to explicitly require that calls to sys_*() system calls made from within the kernel pass through another instrumentation mechanism. IMHO, that would make sense. It would cover both system calls made from kernel threads and system calls made from within a system call or trap.

Mathieu

Well, we can't really set a tracepoint per sys_*() function. Or more precisely we already have them, automagically generated and relying on sysenter ptrace path.

But if we want to check which syscalls are called from kernel threads, we have:

- kthread() -> do_exit()

The entry point of every kernel threads (except "kthreadd") is kthread(). It calls do_exit() in the end.

If we want to trace the exit of a kernel thread, we can put a tracepoint there instead of do_exit() which results would be intermixed with sys_exit() tracing.

- kthreadd :: create_kthread() -> kernel_thread() -> do_fork()

A creation of a thread is the result of the kthreadd thread fork(). If we want to trace the creation of kernel threads, we can again do that in the upper level: kernel_thread().

But does that inform us about who created the thread? All we would see is kthreadd that forks. This is a very poor information compared to a userspace fork() that tells us who really created the new process.

Instead what we want is probably to trace kthread_create() which inserts the job of a thread creation in the kthreadd thread, so that we know _who_ asked for this thread creation (process that requested it and callsite). And that's much more rich in information.

Well, you can even climb in an upper layer and look if this is a workqueue, a kernel/async.c thread, a slow work, etc...

- kernel_execve() -> sys_execve()

We can execute user apps from kernel through call_usermodehelper(). And we can trace kernel_execve() or again in an upper layer like call_usermodehelper()