[PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Steven Rostedt]

Lately I've been testing with an allyesconfig. When I ran the function graph
tracer, it immediately crashed the kernel. Thanks to the new frame pointer
test in function graph, it reported directly what the issue was and then
panicked the kernel to prevent any unexpected damage from happening.

It pointed the error to be with jtcp_rcv_established. Which is a jprobe
function added to tcp_rcv_established at bootup when CONFIG_NET_TCPPROBE
is enabled.

Jprobes and the function graph tracer use the same mechanism to trace
the exit of a function. Unfortunately, only one can be done at a time.
The function graph tracer replaces the return address with its own handler,
but so does jprobes. The two are not compatible.

The solution I am proposing with this patch set is to add a call in
ftrace that lets other code in the kernel permanently disable functions from
being traced by the function and function graph tracer. As a probe function
is registered with jprobes, it calls this new function and that entry
will be removed from being traced.

I tested this with this patch series and it does solve the problem.

Some issues though:

1) this only works when DYNAMIC_FTRACE is enabled. We can prevent
function graph tracing with jprobes when DYNAMIC_FTRACE is not
enabled through Kconfig dependencies. Or have the registering of
a jprobe permanently disable function graph tracing.

2) This also prevents the function tracer from being able to trace a
function probe, even though the function tracer is not at issue
with this bug.

Feedback welcomed.

-- Steve

The following patches are in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: rfc/trace


Steven Rostedt (3):
      tracing: Clean up ftrace.h header and add ftrace_set_notrace() declaration
      tracing: Add calls to permanently disable functions from tracing
      tracing/kprobes: Disable tracing registered jprobe callback functions

----
 include/linux/ftrace.h |   26 +++++++--
 kernel/kprobes.c       |    4 +
 kernel/trace/ftrace.c  |  147 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 150 insertions(+), 27 deletions(-)

[PATCH 1/3][RFC] [PATCH 1/3] tracing: Clean up ftrace.h header and add ftrace_set_notrace() declaration

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-Clean-up-ftrace.h-header-and-add-ftrace_set_.patch --]
[-- Type: text/plain, Size: 2104 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The ftrace_set_notrace() declaration is missing from ftrace.h. This patch
adds it. I also noticed that the #else portion uses macros instead of
inlines, so those are updated as well.

The unregister_ftrace_command() in the #else side had an incorrect
prototype and is now fixed.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace.h |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 0b4f97d..5d31e93 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -158,6 +158,7 @@ struct dyn_ftrace {
 
 int ftrace_force_update(void);
 void ftrace_set_filter(unsigned char *buf, int len, int reset);
+void ftrace_set_notrace(unsigned char *buf, int len, int reset);
 
 int register_ftrace_command(struct ftrace_func_command *cmd);
 int unregister_ftrace_command(struct ftrace_func_command *cmd);
@@ -236,17 +237,26 @@ extern int skip_trace(unsigned long ip);
 extern void ftrace_disable_daemon(void);
 extern void ftrace_enable_daemon(void);
 #else
-# define skip_trace(ip)				({ 0; })
-# define ftrace_force_update()			({ 0; })
-# define ftrace_set_filter(buf, len, reset)	do { } while (0)
-# define ftrace_disable_daemon()		do { } while (0)
-# define ftrace_enable_daemon()			do { } while (0)
+static inline int skip_trace(unsigned long ip)
+{
+	return 0;
+}
+static inline ftrace_force_update(void)
+{
+	return 0;
+}
+static inline void ftrace_disable_daemon(void) { }
+static inline void ftrace_enable_daemon(void) { }
+static inline void
+ftrace_set_filter(unsigned char *buf, int len, int reset) { }
+static inline void
+ftrace_set_notrace(unsigned char *buf, int len, int reset) { }
 static inline void ftrace_release_mod(struct module *mod) {}
 static inline int register_ftrace_command(struct ftrace_func_command *cmd)
 {
 	return -EINVAL;
 }
-static inline int unregister_ftrace_command(char *cmd_name)
+static inline int unregister_ftrace_command(struct ftrace_func_command *cmd);
 {
 	return -EINVAL;
 }
-- 
1.6.3.3

[PATCH 2/3][RFC] [PATCH 2/3] tracing: Add calls to permanently disable functions from tracing

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-Add-calls-to-permanently-disable-functions-f.patch --]
[-- Type: text/plain, Size: 11684 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

There are cases in the kernel where functions need to be dynamically
disabled from tracing. One known instance is with jprobes. Functions
registered as a jprobe will not work with the funtion graph tracer.
This is because the implementation of jprobes and the function graph
tracer collide, and will cause the function graph tracer to panic the
kernel.

This patch adds ftrace_set_disable() and ftrace_disable_function()
to allow other parts of the kernel to mark a function as not to be traced.
These two new functions will permanently disable the function passed in
from being traced by the function and function graph tracer.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace.h |    4 +
 kernel/trace/ftrace.c  |  147 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 130 insertions(+), 21 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 5d31e93..0d518ef 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -159,6 +159,8 @@ struct dyn_ftrace {
 int ftrace_force_update(void);
 void ftrace_set_filter(unsigned char *buf, int len, int reset);
 void ftrace_set_notrace(unsigned char *buf, int len, int reset);
+void ftrace_set_disable(unsigned char *buf, int len);
+void ftrace_disable_function(void *func);
 
 int register_ftrace_command(struct ftrace_func_command *cmd);
 int unregister_ftrace_command(struct ftrace_func_command *cmd);
@@ -251,6 +253,8 @@ static inline void
 ftrace_set_filter(unsigned char *buf, int len, int reset) { }
 static inline void
 ftrace_set_notrace(unsigned char *buf, int len, int reset) { }
+static inline void ftrace_set_disable(unsigned char *buf, int len) { }
+static inline void ftrace_disable_function(void *func) { }
 static inline void ftrace_release_mod(struct module *mod) {}
 static inline int register_ftrace_command(struct ftrace_func_command *cmd)
 {
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 1ed514f..b1d784c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -60,6 +60,18 @@ static int last_ftrace_enabled;
 /* Quick disabling of function tracer. */
 int function_trace_stop;
 
+/*
+ * Flags to pass to regex functions.
+ *  NOTRACE - set the function to not be traced
+ *  FILTER  - set the function to be filtered (only these function are traced)
+ *  DISABLE - disable the function completely (do not let users enable it)
+ */
+enum ftrace_regex {
+	FTRACE_REGEX_NOTRACE		= 0,
+	FTRACE_REGEX_FILTER		= 1,
+	FTRACE_REGEX_DISABLE		= 2,
+};
+
 /* List for set_ftrace_pid's pids. */
 LIST_HEAD(ftrace_pids);
 struct ftrace_pid {
@@ -1602,12 +1614,16 @@ ftrace_failures_open(struct inode *inode, struct file *file)
 }
 
 
-static void ftrace_filter_reset(int enable)
+static void ftrace_filter_reset(enum ftrace_regex enable)
 {
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec;
 	unsigned long type = enable ? FTRACE_FL_FILTER : FTRACE_FL_NOTRACE;
 
+	/* This function should never be called with DISABLE */
+	if (WARN_ON_ONCE(enable == FTRACE_REGEX_DISABLE))
+		return;
+
 	mutex_lock(&ftrace_lock);
 	if (enable)
 		ftrace_filtered = 0;
@@ -1620,11 +1636,16 @@ static void ftrace_filter_reset(int enable)
 }
 
 static int
-ftrace_regex_open(struct inode *inode, struct file *file, int enable)
+ftrace_regex_open(struct inode *inode, struct file *file,
+		  enum ftrace_regex enable)
 {
 	struct ftrace_iterator *iter;
 	int ret = 0;
 
+	/* Users should not be able to permanently disable functions */
+	if (WARN_ON_ONCE(enable == FTRACE_REGEX_DISABLE))
+		return -EINVAL;
+
 	if (unlikely(ftrace_disabled))
 		return -ENODEV;
 
@@ -1665,13 +1686,13 @@ ftrace_regex_open(struct inode *inode, struct file *file, int enable)
 static int
 ftrace_filter_open(struct inode *inode, struct file *file)
 {
-	return ftrace_regex_open(inode, file, 1);
+	return ftrace_regex_open(inode, file, FTRACE_REGEX_FILTER);
 }
 
 static int
 ftrace_notrace_open(struct inode *inode, struct file *file)
 {
-	return ftrace_regex_open(inode, file, 0);
+	return ftrace_regex_open(inode, file, FTRACE_REGEX_NOTRACE);
 }
 
 static loff_t
@@ -1724,7 +1745,8 @@ ftrace_match_record(struct dyn_ftrace *rec, char *regex, int len, int type)
 	return ftrace_match(str, regex, len, type);
 }
 
-static void ftrace_match_records(char *buff, int len, int enable)
+static void ftrace_match_records(char *buff, int len,
+				 enum ftrace_regex enable)
 {
 	unsigned int search_len;
 	struct ftrace_page *pg;
@@ -1734,7 +1756,21 @@ static void ftrace_match_records(char *buff, int len, int enable)
 	int type;
 	int not;
 
-	flag = enable ? FTRACE_FL_FILTER : FTRACE_FL_NOTRACE;
+	switch (enable) {
+	case FTRACE_REGEX_NOTRACE:
+		flag = FTRACE_FL_NOTRACE;
+		break;
+	case FTRACE_REGEX_FILTER:
+		flag = FTRACE_FL_FILTER;
+		break;
+	case FTRACE_REGEX_DISABLE:
+		flag = FTRACE_FL_FAILED;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return;
+	}
+
 	type = filter_parse_regex(buff, len, &search, &not);
 
 	search_len = strlen(search);
@@ -1755,7 +1791,8 @@ static void ftrace_match_records(char *buff, int len, int enable)
 		 * Only enable filtering if we have a function that
 		 * is filtered on.
 		 */
-		if (enable && (rec->flags & FTRACE_FL_FILTER))
+		if (enable == FTRACE_REGEX_FILTER &&
+		    (rec->flags & FTRACE_FL_FILTER))
 			ftrace_filtered = 1;
 	} while_for_each_ftrace_rec();
 	mutex_unlock(&ftrace_lock);
@@ -1763,7 +1800,8 @@ static void ftrace_match_records(char *buff, int len, int enable)
 
 static int
 ftrace_match_module_record(struct dyn_ftrace *rec, char *mod,
-			   char *regex, int len, int type)
+			   char *regex, int len,
+			   enum ftrace_regex enable)
 {
 	char str[KSYM_SYMBOL_LEN];
 	char *modname;
@@ -1775,12 +1813,13 @@ ftrace_match_module_record(struct dyn_ftrace *rec, char *mod,
 
 	/* blank search means to match all funcs in the mod */
 	if (len)
-		return ftrace_match(str, regex, len, type);
+		return ftrace_match(str, regex, len, enable);
 	else
 		return 1;
 }
 
-static void ftrace_match_module_records(char *buff, char *mod, int enable)
+static void ftrace_match_module_records(char *buff, char *mod,
+					enum ftrace_regex enable)
 {
 	unsigned search_len = 0;
 	struct ftrace_page *pg;
@@ -1790,6 +1829,10 @@ static void ftrace_match_module_records(char *buff, char *mod, int enable)
 	unsigned long flag;
 	int not = 0;
 
+	/* this function should not be called for disabling */
+	if (WARN_ON_ONCE(enable == FTRACE_REGEX_DISABLE))
+		return;
+
 	flag = enable ? FTRACE_FL_FILTER : FTRACE_FL_NOTRACE;
 
 	/* blank or '*' mean the same */
@@ -2142,12 +2185,16 @@ int unregister_ftrace_command(struct ftrace_func_command *cmd)
 	return ret;
 }
 
-static int ftrace_process_regex(char *buff, int len, int enable)
+static int ftrace_process_regex(char *buff, int len,
+				enum ftrace_regex enable)
 {
 	char *func, *command, *next = buff;
 	struct ftrace_func_command *p;
 	int ret = -EINVAL;
 
+	if (enable == FTRACE_REGEX_DISABLE)
+		return -EINVAL;
+
 	func = strsep(&next, ":");
 
 	if (!next) {
@@ -2174,7 +2221,7 @@ static int ftrace_process_regex(char *buff, int len, int enable)
 
 static ssize_t
 ftrace_regex_write(struct file *file, const char __user *ubuf,
-		   size_t cnt, loff_t *ppos, int enable)
+		   size_t cnt, loff_t *ppos, enum ftrace_regex enable)
 {
 	struct ftrace_iterator *iter;
 	struct trace_parser *parser;
@@ -2183,6 +2230,9 @@ ftrace_regex_write(struct file *file, const char __user *ubuf,
 	if (!cnt)
 		return 0;
 
+	if (WARN_ON_ONCE(enable == FTRACE_REGEX_DISABLE))
+		return -EINVAL;
+
 	mutex_lock(&ftrace_regex_lock);
 
 	if (file->f_mode & FMODE_READ) {
@@ -2215,27 +2265,35 @@ static ssize_t
 ftrace_filter_write(struct file *file, const char __user *ubuf,
 		    size_t cnt, loff_t *ppos)
 {
-	return ftrace_regex_write(file, ubuf, cnt, ppos, 1);
+	return ftrace_regex_write(file, ubuf, cnt, ppos, FTRACE_REGEX_FILTER);
 }
 
 static ssize_t
 ftrace_notrace_write(struct file *file, const char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
 {
-	return ftrace_regex_write(file, ubuf, cnt, ppos, 0);
+	return ftrace_regex_write(file, ubuf, cnt, ppos, FTRACE_REGEX_NOTRACE);
 }
 
 static void
-ftrace_set_regex(unsigned char *buf, int len, int reset, int enable)
+__ftrace_set_regex(unsigned char *buf, int len, int reset,
+		   enum ftrace_regex enable)
 {
 	if (unlikely(ftrace_disabled))
 		return;
 
-	mutex_lock(&ftrace_regex_lock);
 	if (reset)
 		ftrace_filter_reset(enable);
 	if (buf)
 		ftrace_match_records(buf, len, enable);
+}
+
+static void
+ftrace_set_regex(unsigned char *buf, int len, int reset,
+		 enum ftrace_regex enable)
+{
+	mutex_lock(&ftrace_regex_lock);
+	__ftrace_set_regex(buf, len, reset, enable);
 	mutex_unlock(&ftrace_regex_lock);
 }
 
@@ -2250,7 +2308,7 @@ ftrace_set_regex(unsigned char *buf, int len, int reset, int enable)
  */
 void ftrace_set_filter(unsigned char *buf, int len, int reset)
 {
-	ftrace_set_regex(buf, len, reset, 1);
+	ftrace_set_regex(buf, len, reset, FTRACE_REGEX_FILTER);
 }
 
 /**
@@ -2265,7 +2323,53 @@ void ftrace_set_filter(unsigned char *buf, int len, int reset)
  */
 void ftrace_set_notrace(unsigned char *buf, int len, int reset)
 {
-	ftrace_set_regex(buf, len, reset, 0);
+	ftrace_set_regex(buf, len, reset, FTRACE_REGEX_NOTRACE);
+}
+
+/**
+ * ftrace_set_disable - permanently disable function from tracing
+ * @buf - the string that holds the function to be disabled.
+ * @len - the length of the string.
+ *
+ * This will permanently disable @buf function from ever being
+ * traced.
+ */
+void ftrace_set_disable(unsigned char *buf, int len)
+{
+	if (len < 0)
+		return;
+	mutex_lock(&ftrace_regex_lock);
+	/*
+	 * If function tracer is currently running, we must disable
+	 * the function first using the notrace filter. Otherwise
+	 * we will end up doing the opposite of what we want.
+	 * We would permanently enable the function.
+	 */
+	mutex_lock(&ftrace_lock);
+	if (ftrace_start_up && ftrace_enabled) {
+		__ftrace_set_regex(buf, len, 0, FTRACE_REGEX_NOTRACE);
+		ftrace_run_update_code(FTRACE_ENABLE_CALLS);
+	}
+	mutex_unlock(&ftrace_lock);
+	__ftrace_set_regex(buf, len, 0, FTRACE_REGEX_DISABLE);
+	mutex_unlock(&ftrace_regex_lock);
+ }
+
+/**
+ * ftrace_disable_function - permanently disable a function from tracing
+ * @func - a pointer to the function to be disabled.
+ *
+ * This is called by code that registers functions dynamically that
+ * can cause a problem with tracing. For example, kprobes can use
+ * this to prevent a probe function from being traced.
+ */
+void ftrace_disable_function(void *func)
+{
+	char str[KSYM_SYMBOL_LEN];
+
+	/* Get the name of this function */
+	kallsyms_lookup((unsigned long)func, NULL, NULL, NULL, str);
+	ftrace_set_disable(str, strlen(str));
 }
 
 /*
@@ -2338,7 +2442,8 @@ static void __init set_ftrace_early_filters(void)
 }
 
 static int
-ftrace_regex_release(struct inode *inode, struct file *file, int enable)
+ftrace_regex_release(struct inode *inode, struct file *file,
+		     enum ftrace_regex enable)
 {
 	struct seq_file *m = (struct seq_file *)file->private_data;
 	struct ftrace_iterator *iter;
@@ -2373,13 +2478,13 @@ ftrace_regex_release(struct inode *inode, struct file *file, int enable)
 static int
 ftrace_filter_release(struct inode *inode, struct file *file)
 {
-	return ftrace_regex_release(inode, file, 1);
+	return ftrace_regex_release(inode, file, FTRACE_REGEX_FILTER);
 }
 
 static int
 ftrace_notrace_release(struct inode *inode, struct file *file)
 {
-	return ftrace_regex_release(inode, file, 0);
+	return ftrace_regex_release(inode, file, FTRACE_REGEX_NOTRACE);
 }
 
 static const struct file_operations ftrace_avail_fops = {
-- 
1.6.3.3

[PATCH 3/3][RFC] [PATCH 3/3] tracing/kprobes: Disable tracing registered jprobe callback functions

[Steven Rostedt]

[-- Attachment #1: 0003-tracing-kprobes-Disable-tracing-registered-jprobe-ca.patch --]
[-- Type: text/plain, Size: 1051 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Functions registered with jprobes will cause the function graph tracer
to crash. This is because the implementation of the function graph
tracer and jprobes collide. This patch permanently removes the registered
jprobe function from being traced.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/kprobes.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 5240d75..2589b8a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -42,6 +42,7 @@
 #include <linux/freezer.h>
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
+#include <linux/ftrace.h>
 #include <linux/kdebug.h>
 #include <linux/memory.h>
 
@@ -897,6 +898,9 @@ int __kprobes register_jprobes(struct jprobe **jps, int num)
 		jp = jps[i];
 		addr = arch_deref_entry_point(jp->entry);
 
+		/* This may mess up function graph tracer */
+		ftrace_disable_function(jp->entry);
+
 		if (!kernel_text_address(addr))
 			ret = -EINVAL;
 		else {
-- 
1.6.3.3

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Masami Hiramatsu]

Steven Rostedt wrote:
> Lately I've been testing with an allyesconfig. When I ran the function graph
> tracer, it immediately crashed the kernel. Thanks to the new frame pointer
> test in function graph, it reported directly what the issue was and then
> panicked the kernel to prevent any unexpected damage from happening.
> 
> It pointed the error to be with jtcp_rcv_established. Which is a jprobe
> function added to tcp_rcv_established at bootup when CONFIG_NET_TCPPROBE
> is enabled.
> 
> Jprobes and the function graph tracer use the same mechanism to trace
> the exit of a function. Unfortunately, only one can be done at a time.
> The function graph tracer replaces the return address with its own handler,
> but so does jprobes. The two are not compatible.

AFAIK, Jprobe doesn't trace the exit of a function. I assume that
jprobe's user handler causes the problem, since the handler never
returns normal way.
Instead of that, it just calls jprobe_return() which causes
int3 to be trapped by kprobe's break handler. And the break handler
fixup regs->ip to back to traced function.

Actually, this will cause a problem with function graph tracer.
The f-g-tracer push the return address into the special stack and replaces
it with fixup function (This is similar (not same) mechanism of kretprobe.)
And then the traced function returns, it returns to the fixup function and
it pops the return address up and back to the real caller.

So, if the f-g-tracer traces jprobe user handler, the pop operation
will be skipped because the the handler never returns.

> The solution I am proposing with this patch set is to add a call in
> ftrace that lets other code in the kernel permanently disable functions from
> being traced by the function and function graph tracer. As a probe function
> is registered with jprobes, it calls this new function and that entry
> will be removed from being traced.
>
> I tested this with this patch series and it does solve the problem.
> 
> Some issues though:
> 
> 1) this only works when DYNAMIC_FTRACE is enabled. We can prevent
> function graph tracing with jprobes when DYNAMIC_FTRACE is not
> enabled through Kconfig dependencies. Or have the registering of
> a jprobe permanently disable function graph tracing.

IMHO, those *probe handler should be tagged as __kprobes and notrace.

> 2) This also prevents the function tracer from being able to trace a
> function probe, even though the function tracer is not at issue
> with this bug.

I think we can skip those user handlers, because those are irregular
functions and user can control (enable/disable) it.

BTW, in this specific case, I assume that it can use tracepoint
instead of jprobe and move tcp_probe to a part of ftrace :-), isn't it?
(Or, if it is just for a debugging, Systemtap can help it.)

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Steven Rostedt]

On Thu, 2009-10-29 at 18:02 -0400, Masami Hiramatsu wrote:
> > 
> > Jprobes and the function graph tracer use the same mechanism to trace
> > the exit of a function. Unfortunately, only one can be done at a time.
> > The function graph tracer replaces the return address with its own handler,
> > but so does jprobes. The two are not compatible.
> 
> AFAIK, Jprobe doesn't trace the exit of a function. I assume that
> jprobe's user handler causes the problem, since the handler never
> returns normal way.
> Instead of that, it just calls jprobe_return() which causes
> int3 to be trapped by kprobe's break handler. And the break handler
> fixup regs->ip to back to traced function.

Ah, yes, my documenting this is wrong. It's the skipped jprobe that
messed it up.

> 
> Actually, this will cause a problem with function graph tracer.
> The f-g-tracer push the return address into the special stack and replaces
> it with fixup function (This is similar (not same) mechanism of kretprobe.)
> And then the traced function returns, it returns to the fixup function and
> it pops the return address up and back to the real caller.
> 
> So, if the f-g-tracer traces jprobe user handler, the pop operation
> will be skipped because the the handler never returns.

Exactly!

> 
> > The solution I am proposing with this patch set is to add a call in
> > ftrace that lets other code in the kernel permanently disable functions from
> > being traced by the function and function graph tracer. As a probe function
> > is registered with jprobes, it calls this new function and that entry
> > will be removed from being traced.
> >
> > I tested this with this patch series and it does solve the problem.
> > 
> > Some issues though:
> > 
> > 1) this only works when DYNAMIC_FTRACE is enabled. We can prevent
> > function graph tracing with jprobes when DYNAMIC_FTRACE is not
> > enabled through Kconfig dependencies. Or have the registering of
> > a jprobe permanently disable function graph tracing.
> 
> IMHO, those *probe handler should be tagged as __kprobes and notrace.

Yeah, I agree. But how do you guarantee that it does. If one forgets,
than we still have the issue. We can perhaps test to make sure the
function is in the kprobes section. But that does not mean they will not
be notraced. The __kprobes and notrace are no longer in the same set.

> 
> > 2) This also prevents the function tracer from being able to trace a
> > function probe, even though the function tracer is not at issue
> > with this bug.
> 
> I think we can skip those user handlers, because those are irregular
> functions and user can control (enable/disable) it.

True, but it may be nice to still trace them.

> 
> BTW, in this specific case, I assume that it can use tracepoint
> instead of jprobe and move tcp_probe to a part of ftrace :-), isn't it?
> (Or, if it is just for a debugging, Systemtap can help it.)

That's a question for the networking guys.

-- Steve

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Stephen Hemminger]

On Thu, 29 Oct 2009 18:17:33 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 2009-10-29 at 18:02 -0400, Masami Hiramatsu wrote:
> > > 
> > > Jprobes and the function graph tracer use the same mechanism to trace
> > > the exit of a function. Unfortunately, only one can be done at a time.
> > > The function graph tracer replaces the return address with its own handler,
> > > but so does jprobes. The two are not compatible.
> > 
> > AFAIK, Jprobe doesn't trace the exit of a function. I assume that
> > jprobe's user handler causes the problem, since the handler never
> > returns normal way.
> > Instead of that, it just calls jprobe_return() which causes
> > int3 to be trapped by kprobe's break handler. And the break handler
> > fixup regs->ip to back to traced function.
> 
> Ah, yes, my documenting this is wrong. It's the skipped jprobe that
> messed it up.
> 
> > 
> > Actually, this will cause a problem with function graph tracer.
> > The f-g-tracer push the return address into the special stack and replaces
> > it with fixup function (This is similar (not same) mechanism of kretprobe.)
> > And then the traced function returns, it returns to the fixup function and
> > it pops the return address up and back to the real caller.
> > 
> > So, if the f-g-tracer traces jprobe user handler, the pop operation
> > will be skipped because the the handler never returns.
> 
> Exactly!
> 
> > 
> > > The solution I am proposing with this patch set is to add a call in
> > > ftrace that lets other code in the kernel permanently disable functions from
> > > being traced by the function and function graph tracer. As a probe function
> > > is registered with jprobes, it calls this new function and that entry
> > > will be removed from being traced.
> > >
> > > I tested this with this patch series and it does solve the problem.
> > > 
> > > Some issues though:
> > > 
> > > 1) this only works when DYNAMIC_FTRACE is enabled. We can prevent
> > > function graph tracing with jprobes when DYNAMIC_FTRACE is not
> > > enabled through Kconfig dependencies. Or have the registering of
> > > a jprobe permanently disable function graph tracing.
> > 
> > IMHO, those *probe handler should be tagged as __kprobes and notrace.
> 
> Yeah, I agree. But how do you guarantee that it does. If one forgets,
> than we still have the issue. We can perhaps test to make sure the
> function is in the kprobes section. But that does not mean they will not
> be notraced. The __kprobes and notrace are no longer in the same set.
> 
> > 
> > > 2) This also prevents the function tracer from being able to trace a
> > > function probe, even though the function tracer is not at issue
> > > with this bug.
> > 
> > I think we can skip those user handlers, because those are irregular
> > functions and user can control (enable/disable) it.
> 
> True, but it may be nice to still trace them.
> 
> > 
> > BTW, in this specific case, I assume that it can use tracepoint
> > instead of jprobe and move tcp_probe to a part of ftrace :-), isn't it?
> > (Or, if it is just for a debugging, Systemtap can help it.)
> 
> That's a question for the networking guys.
> 

tcp_probe is simple tool used for research graphs, there are scripts
and stuff wrapped around it. If you keep the ABI, go ahead and convert
it to ftrace.

-- 

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Masami Hiramatsu]

Steven Rostedt wrote:
> On Thu, 2009-10-29 at 18:02 -0400, Masami Hiramatsu wrote:\
>>> The solution I am proposing with this patch set is to add a call in
>>> ftrace that lets other code in the kernel permanently disable functions from
>>> being traced by the function and function graph tracer. As a probe function
>>> is registered with jprobes, it calls this new function and that entry
>>> will be removed from being traced.
>>>
>>> I tested this with this patch series and it does solve the problem.
>>>
>>> Some issues though:
>>>
>>> 1) this only works when DYNAMIC_FTRACE is enabled. We can prevent
>>> function graph tracing with jprobes when DYNAMIC_FTRACE is not
>>> enabled through Kconfig dependencies. Or have the registering of
>>> a jprobe permanently disable function graph tracing.
>>
>> IMHO, those *probe handler should be tagged as __kprobes and notrace.
> 
> Yeah, I agree. But how do you guarantee that it does. If one forgets,
> than we still have the issue. We can perhaps test to make sure the
> function is in the kprobes section. But that does not mean they will not
> be notraced. The __kprobes and notrace are no longer in the same set.
> 
>>
>>> 2) This also prevents the function tracer from being able to trace a
>>> function probe, even though the function tracer is not at issue
>>> with this bug.
>>
>> I think we can skip those user handlers, because those are irregular
>> functions and user can control (enable/disable) it.
> 
> True, but it may be nice to still trace them.

Hm, in that case, I think we can change jprobe_return() to call
f-g-tracer's return handler if needed as below;
---
static inline jprobe_return(void)
{
	implicit_function_return();	/* This executes f-g-tracer prologue */
	__jprobe_return();	/* This should be notraced */
}
---

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Steven Rostedt]

On Thu, 2009-10-29 at 19:22 -0400, Masami Hiramatsu wrote:
> Steven Rostedt wrote:

> >> I think we can skip those user handlers, because those are irregular
> >> functions and user can control (enable/disable) it.
> > 
> > True, but it may be nice to still trace them.
> 
> Hm, in that case, I think we can change jprobe_return() to call
> f-g-tracer's return handler if needed as below;
> ---
> static inline jprobe_return(void)
> {
> 	implicit_function_return();	/* This executes f-g-tracer prologue */
> 	__jprobe_return();	/* This should be notraced */
> }

Hmm, That looks like it can be quite a hack. We don't know at that
moment if the handler has been traced or not. We can't always do the
function graph logic.

-- Steve

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Masami Hiramatsu]

Steven Rostedt wrote:
> On Thu, 2009-10-29 at 19:22 -0400, Masami Hiramatsu wrote:
>> Steven Rostedt wrote:
> 
>>>> I think we can skip those user handlers, because those are irregular
>>>> functions and user can control (enable/disable) it.
>>>
>>> True, but it may be nice to still trace them.
>>
>> Hm, in that case, I think we can change jprobe_return() to call
>> f-g-tracer's return handler if needed as below;
>> ---
>> static inline jprobe_return(void)
>> {
>> 	implicit_function_return();	/* This executes f-g-tracer prologue */
>> 	__jprobe_return();	/* This should be notraced */
>> }
> 
> Hmm, That looks like it can be quite a hack. We don't know at that
> moment if the handler has been traced or not. We can't always do the
> function graph logic.

Ah, right. Perhaps, jprobe might be able to save the address of top
of the stack and check if it has been modified to return_to_handler
at jprobe_return().

But anyway, I think your patch can fix that too. :-)

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Frank Ch. Eigler]

Steven Rostedt <rostedt@goodmis.org> writes:

> [...]  Jprobes and the function graph tracer use the same mechanism
> to trace the exit of a function. Unfortunately, only one can be done
> at a time.  The function graph tracer replaces the return address
> with its own handler, but so does jprobes. The two are not
> compatible. [...]

What about kretprobes?  It too uses the same mechanism.

- FChE

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Masami Hiramatsu]

Frank Ch. Eigler wrote:
> Steven Rostedt<rostedt@goodmis.org>  writes:
>
>> [...]  Jprobes and the function graph tracer use the same mechanism
>> to trace the exit of a function. Unfortunately, only one can be done
>> at a time.  The function graph tracer replaces the return address
>> with its own handler, but so does jprobes. The two are not
>> compatible. [...]
>
> What about kretprobes?  It too uses the same mechanism.

Exactly, kretprobe uses similar mechanism with func-graph tracer.

Fortunately, it doesn't cause any problem, because kretprobe doesn't
skip any function-return, and func-graph tracer(mcount) always intrudes
a function after kretprobe.

---
call func
	<-- kretprobe prehandler changes return address
	func()
	<-- func-graph prehandler changes return address
	...
	return
	--> func-graph trampoline sets ip to "kretprobe trampoline"
	--> kretprobe trampoline sets ip to "real return address"
(return address)
---

So that kretprobe handler doesn't conflict with function graph tracer.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Frederic Weisbecker]

On Thu, Oct 29, 2009 at 06:02:20PM -0400, Masami Hiramatsu wrote:
> Steven Rostedt wrote:
> > Lately I've been testing with an allyesconfig. When I ran the function graph
> > tracer, it immediately crashed the kernel. Thanks to the new frame pointer
> > test in function graph, it reported directly what the issue was and then
> > panicked the kernel to prevent any unexpected damage from happening.
> > 
> > It pointed the error to be with jtcp_rcv_established. Which is a jprobe
> > function added to tcp_rcv_established at bootup when CONFIG_NET_TCPPROBE
> > is enabled.
> > 
> > Jprobes and the function graph tracer use the same mechanism to trace
> > the exit of a function. Unfortunately, only one can be done at a time.
> > The function graph tracer replaces the return address with its own handler,
> > but so does jprobes. The two are not compatible.
> 
> AFAIK, Jprobe doesn't trace the exit of a function. I assume that
> jprobe's user handler causes the problem, since the handler never
> returns normal way.
> Instead of that, it just calls jprobe_return() which causes
> int3 to be trapped by kprobe's break handler. And the break handler
> fixup regs->ip to back to traced function.
> 
> Actually, this will cause a problem with function graph tracer.
> The f-g-tracer push the return address into the special stack and replaces
> it with fixup function (This is similar (not same) mechanism of kretprobe.)
> And then the traced function returns, it returns to the fixup function and
> it pops the return address up and back to the real caller.
> 
> So, if the f-g-tracer traces jprobe user handler, the pop operation
> will be skipped because the the handler never returns.


I'm not sure I've well understood how is performed the call to the jprobe
handler.
But if I understand well we have:

	func() {
		int3() {
			jprobe_handler() {
				(-)
				set ip after iret to user_handler()
			}
		}
		user_handler() {
			jprobe_return() {
				(+)
				int3() {
					set ip after iret to func+...()
				}
			       |
			       |
			       |
                <--------------
                (execute the rest of func())
	}

If we replace (-) with pause_graph_tracing() and (+) with
unpause_graph_tracing(), this should do the trick...I hope.

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Masami Hiramatsu]

Frederic Weisbecker wrote:
> On Thu, Oct 29, 2009 at 06:02:20PM -0400, Masami Hiramatsu wrote:
>> Steven Rostedt wrote:
>>> Lately I've been testing with an allyesconfig. When I ran the function graph
>>> tracer, it immediately crashed the kernel. Thanks to the new frame pointer
>>> test in function graph, it reported directly what the issue was and then
>>> panicked the kernel to prevent any unexpected damage from happening.
>>>
>>> It pointed the error to be with jtcp_rcv_established. Which is a jprobe
>>> function added to tcp_rcv_established at bootup when CONFIG_NET_TCPPROBE
>>> is enabled.
>>>
>>> Jprobes and the function graph tracer use the same mechanism to trace
>>> the exit of a function. Unfortunately, only one can be done at a time.
>>> The function graph tracer replaces the return address with its own handler,
>>> but so does jprobes. The two are not compatible.
>>
>> AFAIK, Jprobe doesn't trace the exit of a function. I assume that
>> jprobe's user handler causes the problem, since the handler never
>> returns normal way.
>> Instead of that, it just calls jprobe_return() which causes
>> int3 to be trapped by kprobe's break handler. And the break handler
>> fixup regs->ip to back to traced function.
>>
>> Actually, this will cause a problem with function graph tracer.
>> The f-g-tracer push the return address into the special stack and replaces
>> it with fixup function (This is similar (not same) mechanism of kretprobe.)
>> And then the traced function returns, it returns to the fixup function and
>> it pops the return address up and back to the real caller.
>>
>> So, if the f-g-tracer traces jprobe user handler, the pop operation
>> will be skipped because the the handler never returns.
>
>
> I'm not sure I've well understood how is performed the call to the jprobe
> handler.
> But if I understand well we have:
>
> 	func() {
> 		int3() {
> 			jprobe_handler() {
> 				(-)
> 				set ip after iret to user_handler()
> 			}
> 		}
> 		user_handler() {
> 			jprobe_return() {
> 				(+)
> 				int3() {
> 					set ip after iret to func+...()
> 				}
> 			       |
> 			       |
> 			       |
>                  <--------------
>                  (execute the rest of func())
> 	}
>
> If we replace (-) with pause_graph_tracing() and (+) with
> unpause_graph_tracing(), this should do the trick...I hope.

I'm not so sure about pause_graph_tracing(), however, it seems that
int3() and jprobe_handler() already pushed on the stack of the
func graph tracer at (-). If it's true, where are those entries
popped up?

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Frederic Weisbecker]

On Mon, Nov 02, 2009 at 10:02:23AM -0500, Masami Hiramatsu wrote:
> Frederic Weisbecker wrote:
>> I'm not sure I've well understood how is performed the call to the jprobe
>> handler.
>> But if I understand well we have:
>>
>> 	func() {
>> 		int3() {
>> 			jprobe_handler() {
>> 				(-)
>> 				set ip after iret to user_handler()
>> 			}
>> 		}
>> 		user_handler() {
>> 			jprobe_return() {
>> 				(+)
>> 				int3() {
>> 					set ip after iret to func+...()
>> 				}
>> 			       |
>> 			       |
>> 			       |
>>                  <--------------
>>                  (execute the rest of func())
>> 	}
>>
>> If we replace (-) with pause_graph_tracing() and (+) with
>> unpause_graph_tracing(), this should do the trick...I hope.
>
> I'm not so sure about pause_graph_tracing(), however, it seems that
> int3() and jprobe_handler() already pushed on the stack of the
> func graph tracer at (-). If it's true, where are those entries
> popped up?
>


pause_graph_tracing() will disable the tracing for the current task
but it won't disable the address pop from stack.

If the above jprobe scheme is right, the scenario will be:

      func() {
              /* push func ret */
              int3() {
                      /* push do_trap ret */
                      jprobe_handler() {
                              /* push jprobe_handler ret */
                              pause_graph_tracing();
                              set ip after iret to user_handler()
                      } /* pop jprobe_handler ret */
              } /* pop do_trap ret */
              user_handler() {
                      jprobe_return() {
                              unpause_graph_tracing()
                              int3() {
                                     /* push do_trap ret */
                                      set ip after iret to func+...()
                              } /* pop do_trap ret */
                             |
                             |
                             |
               <--------------
               (execute the rest of func())
      } /* pop func ret */


Hmm?

Re: [PATCH 0/3][RFC] tracing/kprobes: prevent jprobes from crashing function graph tracer

[Masami Hiramatsu]

Frederic Weisbecker wrote:
> On Mon, Nov 02, 2009 at 10:02:23AM -0500, Masami Hiramatsu wrote:
>> Frederic Weisbecker wrote:
>>> I'm not sure I've well understood how is performed the call to the jprobe
>>> handler.
>>> But if I understand well we have:
>>>
>>> 	func() {
>>> 		int3() {
>>> 			jprobe_handler() {
>>> 				(-)
>>> 				set ip after iret to user_handler()
>>> 			}
>>> 		}
>>> 		user_handler() {
>>> 			jprobe_return() {
>>> 				(+)
>>> 				int3() {
>>> 					set ip after iret to func+...()
>>> 				}
>>> 			       |
>>> 			       |
>>> 			       |
>>>                   <--------------
>>>                   (execute the rest of func())
>>> 	}
>>>
>>> If we replace (-) with pause_graph_tracing() and (+) with
>>> unpause_graph_tracing(), this should do the trick...I hope.
>>
>> I'm not so sure about pause_graph_tracing(), however, it seems that
>> int3() and jprobe_handler() already pushed on the stack of the
>> func graph tracer at (-). If it's true, where are those entries
>> popped up?
>>
>
>
> pause_graph_tracing() will disable the tracing for the current task
> but it won't disable the address pop from stack.
>
> If the above jprobe scheme is right, the scenario will be:
>
>        func() {
>                /* push func ret */
>                int3() {
>                        /* push do_trap ret */
>                        jprobe_handler() {
>                                /* push jprobe_handler ret */
>                                pause_graph_tracing();
>                                set ip after iret to user_handler()
>                        } /* pop jprobe_handler ret */
>                } /* pop do_trap ret */
>                user_handler() {
>                        jprobe_return() {
>                                unpause_graph_tracing()
>                                int3() {
>                                       /* push do_trap ret */
>                                        set ip after iret to func+...()
>                                } /* pop do_trap ret */
>                               |
>                               |
>                               |
>                 <--------------
>                 (execute the rest of func())
>        } /* pop func ret */
>
>
> Hmm?

Oh, I see. That should work. :-)

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com