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

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

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

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

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

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

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

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

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

- ... I guess there are other examples

The kernel calls syscalls through wrappers, and tracing these wrappers, depending of the desired level of informations we want (choose your layer), are much more verbose / rich in informations.

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

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

Quoted 6 lines

- ... I guess there are other examples

The kernel calls syscalls through wrappers, and tracing these wrappers, depending of the desired level of informations we want (choose your layer), are much more verbose / rich in informations.

What you describe looks a lot like the approach I use in the LTTng tree. Actually, the main point I am trying to make here is: if we rely only on tracing at the syscall entry/exit level for, say, monitoring all uses of e.g. sys_open(), we might be caught offguard by internal sys_open() uses within the kernel.

Quoted 123 lines · Mathieu Desnoyers

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

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

Quoted 6 lines

- ... I guess there are other examples

The kernel calls syscalls through wrappers, and tracing these wrappers, depending of the desired level of informations we want (choose your layer), are much more verbose / rich in informations.

Quoted 5 lines

What you describe looks a lot like the approach I use in the LTTng tree. Actually, the main point I am trying to make here is: if we rely only on tracing at the syscall entry/exit level for, say, monitoring all uses of e.g. sys_open(), we might be caught offguard by internal sys_open() uses within the kernel.

There's a lot of 'internal' file opening going on within the kernel that ptrace does not notice - see all the filp_open() calls.

Lets worry about this only if it's a true issue.

Ingo

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 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 5 lines · On Tue, 25 Aug 2009 14:31:19 -0400 Mathieu Desnoyers…

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

I guess that the real reason for the crash is hidden in the initialization of the pt_regs structure of the kernel thread.

Quoted 4 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.

That is my best guess as well.

Quoted 7 lines · Martin Schwidefsky

Quoted 5 lines · On Tue, 25 Aug 2009 14:31:19 -0400 Mathieu Desnoyers…

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.

That's broken.. some proprietary drivers even change the system call table. Do you want to be able to deal with that as well?

Quoted 6 lines

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

I guess that the real reason for the crash is hidden in the initialization of the pt_regs structure of the kernel thread.

On s390 the reason is that the scvnr in the pt_regs structure of the initial kernel thread is initialized to 0. svcnr contains the system call number and system call number 0 does not exist.
That's why we have

static inline long syscall_get_nr(struct task_struct *task,
struct pt_regs *regs)

{
	return regs->svcnr ? regs->svcnr : -1;
}

Now, if you fork a kernel thread from the initial task the pt_regs structure gets copied. Upon ret_from_fork the trace exit path will get -1 for syscall_get_nr().

Quoted 6 lines

Quoted 4 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.

That is my best guess as well.

What would that value be? __NR_fork?

Syscall tracing of kernel threads seems to be wrong. If somebody would do a "modprobe" and the init function of the module would create a kernel thread then syscall_get_nr() at the ret_from_fork path of the kernel thread would return __NR_init_module. That is of course only true if the old kernel_thread() API would be used. For kthread_create() it would return the syscall of the thread from which the kthread daemon was forked (the initial process I would guess, which was initialized to 0).

So skipping kernel threads at the exit path seems so be the best fix, IMHO ;)

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

Index: linux-next/kernel/trace/trace_syscalls.c
===================================================================

kernel/trace/trace_syscalls.c +0 -0

 	syscall_nr = syscall_get_nr(current, regs);
 	if (!test_bit(syscall_nr, enabled_exit_syscalls))
 		return;

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

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.

Quoted 4 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.

So you're saying we should let proprietary crap influence the design of the kernel in any way?

Nah. And I start to feel comfortable with syscall entry/exit being only be traced for userspace threads. But as I pointed out in a follow-up email, the lack of sys_*() tracing for invocation from within the kernel might be problematic. This is actually my main point.

We do not support system calls from kernelspace anymore. All the macros to do real system calls are gone, and there are very very few places left calling sys_foo as normal function calls.

An how exactly is calling sys_foo as a normal function call different from calling do_foo or vfs_foo?

Quoted 6 lines · Christoph Hellwig

We do not support system calls from kernelspace anymore. All the macros to do real system calls are gone, and there are very very few places left calling sys_foo as normal function calls.

An how exactly is calling sys_foo as a normal function call different from calling do_foo or vfs_foo?

And if you really need to trace direct callers of sys_foo just put a jprobe on it.

From userland:

From the kernel:

For instance, some
proprietary Linux driver does very odd things with system calls within kernel threads, like invoking them with int 0x80.

Quoted 3 lines · Mathieu Desnoyers

For instance, some
proprietary Linux driver does very odd things with system calls within kernel threads, like invoking them with int 0x80.

So who is going to send the x86 patch to make int 0x80 from kernel space panic the machine? :-)

Quoted 3 lines · Mathieu Desnoyers

For instance, some
proprietary Linux driver does very odd things with system calls within kernel threads, like invoking them with int 0x80.

So who is going to send the x86 patch to make int 0x80 from kernel space panic the machine? :-)

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 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 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 40 lines · Hendrik Brueckner

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 601

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);
 		} while_each_thread(g, t);
 		read_unlock_irqrestore(&tasklist_lock, flags);
 	}

Yeah, and as told before, syscalls tracing from kernel thread is an interesting point but we can't do it that way.

I'm queuing this patch for .32, but I need you Signed-off-by to apply it :)

Quoted 55 lines · Frederic Weisbecker

Quoted 40 lines · Hendrik Brueckner

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 601

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);
 		} while_each_thread(g, t);
 		read_unlock_irqrestore(&tasklist_lock, flags);
 	}

Yeah, and as told before, syscalls tracing from kernel thread is an interesting point but we can't do it that way.

Quoted 1 line

I'm queuing this patch for .32, but I need you Signed-off-by to apply it :)

That won't always work as pointed out in the other example:
- Process doing sys_init_module then scheduled away
- User enables syscall tracing -> TIF_SYSCALL_FTRACE gets set
- init function of the module gets called and is doing kernel_thread() (old API) -> kernel thread inherits TIF_SYSCALL_FTRACE.

I don't think that's what you want. You might want to clear the flag for new processes during fork (only for kernel threads I would guess).

At least the current patch leaves a hole.

Quoted 10 lines

That won't always work as pointed out in the other example:
- Process doing sys_init_module then scheduled away
- User enables syscall tracing -> TIF_SYSCALL_FTRACE gets set
- init function of the module gets called and is doing kernel_thread() (old API) -> kernel thread inherits TIF_SYSCALL_FTRACE.

I don't think that's what you want. You might want to clear the flag for new processes during fork (only for kernel threads I would guess).

Quoted 1 line

At least the current patch leaves a hole.

Ah, there are callsites that use kernel_thread() directly? Does it means that t->mm could be non NULL for such resulting kernel threads, in that case it would be hard to hook on do_fork() to check that.

Quoted 10 lines · Frederic Weisbecker

Quoted 10 lines

That won't always work as pointed out in the other example:
- Process doing sys_init_module then scheduled away
- User enables syscall tracing -> TIF_SYSCALL_FTRACE gets set
- init function of the module gets called and is doing kernel_thread() (old API) -> kernel thread inherits TIF_SYSCALL_FTRACE.

I don't think that's what you want. You might want to clear the flag for new processes during fork (only for kernel threads I would guess).

Quoted 7 lines

Quoted 1 line

At least the current patch leaves a hole.

Ah, there are callsites that use kernel_thread() directly? Does it means that t->mm could be non NULL for such resulting kernel threads, in that case it would be hard to hook on do_fork() to check that.

All kernel threads have a NULL t->mm. Since do_fork is called by kthreadd and not by kthread_create, the caller of do_fork will also have a t->mm = NULL.

-- Steve

Quoted 7 lines

All kernel threads have a NULL t->mm. Since do_fork is called by kthreadd and not by kthread_create, the caller of do_fork will also have a t->mm = NULL.

-- Steve

Yeah, that's the case with kthread_create() creation fashion, but what if you create a kernel thread using the low level kernel_thread() directly (ie: without relaying on kthreadd queue)?

Especially in Heiko example, it seems to be a duplication of user task.

I wonder what obvious think I'm missing here...

Quoted 13 lines · Frederic Weisbecker

Quoted 11 lines

That won't always work as pointed out in the other example:
- Process doing sys_init_module then scheduled away
- User enables syscall tracing -> TIF_SYSCALL_FTRACE gets set
- init function of the module gets called and is doing kernel_thread() (old API) -> kernel thread inherits TIF_SYSCALL_FTRACE.

I don't think that's what you want. You might want to clear the flag for new processes during fork (only for kernel threads I would guess).

At least the current patch leaves a hole.

Quoted 4 lines

Ah, there are callsites that use kernel_thread() directly? Does it means that t->mm could be non NULL for such resulting kernel threads, in that case it would be hard to hook on do_fork() to check that.

All kernel threads have a NULL t->mm. Since do_fork is called by kthreadd and not by kthread_create, the caller of do_fork will also have a t->mm = NULL.

Quoted 19 lines · Steven Rostedt

Quoted 17 lines · Frederic Weisbecker

Quoted 11 lines

That won't always work as pointed out in the other example:
- Process doing sys_init_module then scheduled away
- User enables syscall tracing -> TIF_SYSCALL_FTRACE gets set
- init function of the module gets called and is doing kernel_thread() (old API) -> kernel thread inherits TIF_SYSCALL_FTRACE.

I don't think that's what you want. You might want to clear the flag for new processes during fork (only for kernel threads I would guess).

At least the current patch leaves a hole.

Ah, there are callsites that use kernel_thread() directly? Does it means that t->mm could be non NULL for such resulting kernel threads, in that case it would be hard to hook on do_fork() to check that.

Quoted 3 lines

All kernel threads have a NULL t->mm. Since do_fork is called by kthreadd and not by kthread_create, the caller of do_fork will also have a t->mm = NULL.

Weren't there a few sites in the kernel where kernel threads temporarily borrow the mm from someone?

Quoted 14 lines · Heiko Carstens

Quoted 4 lines

Yeah, and as told before, syscalls tracing from kernel thread is an interesting point but we can't do it that way.

I'm queuing this patch for .32, but I need you Signed-off-by to apply it :)

That won't always work as pointed out in the other example:
- Process doing sys_init_module then scheduled away
- User enables syscall tracing -> TIF_SYSCALL_FTRACE gets set
- init function of the module gets called and is doing kernel_thread() (old API) -> kernel thread inherits TIF_SYSCALL_FTRACE.

I don't think that's what you want. You might want to clear the flag for new processes during fork (only for kernel threads I would guess).

Quoted 1 line

At least the current patch leaves a hole.

Ah, there are callsites that use kernel_thread() directly? Does it means that t->mm could be non NULL for such resulting kernel threads, in that case it would be hard to hook on do_fork() to check that.

Quoted 15 lines · Frederic Weisbecker

Quoted 14 lines · Heiko Carstens

Quoted 4 lines

Yeah, and as told before, syscalls tracing from kernel thread is an interesting point but we can't do it that way.

I'm queuing this patch for .32, but I need you Signed-off-by to apply it :)

That won't always work as pointed out in the other example:
- Process doing sys_init_module then scheduled away
- User enables syscall tracing -> TIF_SYSCALL_FTRACE gets set
- init function of the module gets called and is doing kernel_thread() (old API) -> kernel thread inherits TIF_SYSCALL_FTRACE.

I don't think that's what you want. You might want to clear the flag for new processes during fork (only for kernel threads I would guess).

Quoted 6 lines

Quoted 1 line

At least the current patch leaves a hole.

Ah, there are callsites that use kernel_thread() directly? Does it means that t->mm could be non NULL for such resulting kernel threads, in that case it would be hard to hook on do_fork() to check that.

Oh yes, you are right. kernel threads created with kernel_thread() have t->mm != NULL if the forking process has an mm too.

There are very few callsites left which still use kernel_thread(). (the last one in s390 driver code will be gone after the next merge window).

As far as I can there are only four callsites left (excluding staging): jffs2 and three in net/bluetooth/*

Quoted 10 lines

Oh yes, you are right. kernel threads created with kernel_thread() have t->mm != NULL if the forking process has an mm too.

There are very few callsites left which still use kernel_thread(). (the last one in s390 driver code will be gone after the next merge window).

As far as I can there are only four callsites left (excluding staging): jffs2 and three in net/bluetooth/*

In that case, I'd suggest to pick the patch that checks for kernel threads while setting the TIF_FLAGS.

The check for invalid syscall numbers in another patch should be sufficient to not crash.

We might have rare inaccurate result because of the remaining buggy callsites but instead of working around on it, these should be fixed.

Quoted 18 lines · Frederic Weisbecker

Quoted 10 lines

Oh yes, you are right. kernel threads created with kernel_thread() have t->mm != NULL if the forking process has an mm too.

There are very few callsites left which still use kernel_thread(). (the last one in s390 driver code will be gone after the next merge window).

As far as I can there are only four callsites left (excluding staging): jffs2 and three in net/bluetooth/*

In that case, I'd suggest to pick the patch that checks for kernel threads while setting the TIF_FLAGS.

The check for invalid syscall numbers in another patch should be sufficient to not crash.

Quoted 3 lines

We might have rare inaccurate result because of the remaining buggy callsites but instead of working around on it, these should be fixed.

Well, do these (buggy threads) call syscalls?

-- Steve

Quoted 4 lines

Ah, there are callsites that use kernel_thread() directly? Does it means that t->mm could be non NULL for such resulting kernel threads, in that case it would be hard to hook on do_fork() to check that.

Oh yes, you are right. kernel threads created with kernel_thread() have t->mm != NULL if the forking process has an mm too.

There are very few callsites left which still use kernel_thread(). (the last one in s390 driver code will be gone after the next merge window).

As far as I can there are only four callsites left (excluding staging): jffs2 and three in net/bluetooth/*

Quoted 41 lines · Hendrik Brueckner

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 601

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);
 		} while_each_thread(g, t);
 		read_unlock_irqrestore(&tasklist_lock, flags);
 	}

Yeah, and as told before, syscalls tracing from kernel thread is an interesting point but we can't do it that way.

I'm queuing this patch for .32, but I need you Signed-off-by to apply it :)

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 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 32 lines · Frederic Weisbecker

Quoted 28 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.

[...]

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

Quoted 3 lines

I'm queueing this one for .32

Thanks.

Btw it would be nice to have an ack from s390 maintainers. Martin, Heiko, no problem with this patch?