[PATCH] tracing/function-graph-tracer: fix a regression while suspend to disk

[PATCH] tracing/function-graph-tracer: fix a regression while suspend to disk

[Frederic Weisbecker]

Impact: fix a crash while kernel image restore

When the function graph tracer is running and while suspend to disk, some racy
and dangerous things happen against this tracer.

The current task will save its registers including the stack pointer which
contains the return address hooked by the tracer. But the current task will
continue to enter other functions after that to save the memory, and then
it will store other return addresses, and finally loose the old depth which
matches the return address saved in the old stack (during the registers saving).

So on image restore, the code will return to wrong addresses.
And there are other things: on restore, the task will have it's "current"
pointer overwritten during registers restoring....switching from one task to
another... That would be insane to try to trace function graphs at these
stages.

This patch makes the function graph tracer listening on power events, making
it's tracing disabled for the current task (the one that performs the hibernation work)
while suspend/resume to disk, making the tracing safe during hibernation.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/ftrace.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 8c1c9c0..7e9a20b 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -17,6 +17,7 @@
 #include <linux/clocksource.h>
 #include <linux/kallsyms.h>
 #include <linux/seq_file.h>
+#include <linux/suspend.h>
 #include <linux/debugfs.h>
 #include <linux/hardirq.h>
 #include <linux/kthread.h>
@@ -1957,6 +1958,7 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
 static atomic_t ftrace_graph_active;
+static struct notifier_block ftrace_suspend_notifier;
 
 int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
 {
@@ -2035,6 +2037,27 @@ static int start_graph_tracing(void)
 	return ret;
 }
 
+/*
+ * Hibernation protection.
+ * The state of the current task is too much unstable during
+ * suspend/restore to disk. We want to protect against that.
+ */
+static int
+ftrace_suspend_notifier_call(struct notifier_block *bl, unsigned long state,
+							void *unused)
+{
+	switch (state) {
+	case PM_HIBERNATION_PREPARE:
+		pause_graph_tracing();
+		break;
+
+	case PM_POST_HIBERNATION:
+		unpause_graph_tracing();
+		break;
+	}
+	return NOTIFY_DONE;
+}
+
 int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 			trace_func_graph_ent_t entryfunc)
 {
@@ -2042,6 +2065,9 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 
 	mutex_lock(&ftrace_sysctl_lock);
 
+	ftrace_suspend_notifier.notifier_call = ftrace_suspend_notifier_call;
+	register_pm_notifier(&ftrace_suspend_notifier);
+
 	atomic_inc(&ftrace_graph_active);
 	ret = start_graph_tracing();
 	if (ret) {
@@ -2067,6 +2093,7 @@ void unregister_ftrace_graph(void)
 	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
 	ftrace_graph_entry = ftrace_graph_entry_stub;
 	ftrace_shutdown(FTRACE_STOP_FUNC_RET);
+	unregister_pm_notifier(&ftrace_suspend_notifier);
 
 	mutex_unlock(&ftrace_sysctl_lock);
 }
-- 
1.6.0.4

Re: [PATCH] tracing/function-graph-tracer: fix a regression while suspend to disk

[Steven Rostedt]

On Wed, 14 Jan 2009, Frederic Weisbecker wrote:

> Impact: fix a crash while kernel image restore
> 
> When the function graph tracer is running and while suspend to disk, some racy
> and dangerous things happen against this tracer.
> 
> The current task will save its registers including the stack pointer which
> contains the return address hooked by the tracer. But the current task will
> continue to enter other functions after that to save the memory, and then
> it will store other return addresses, and finally loose the old depth which
> matches the return address saved in the old stack (during the registers saving).
> 
> So on image restore, the code will return to wrong addresses.
> And there are other things: on restore, the task will have it's "current"
> pointer overwritten during registers restoring....switching from one task to
> another... That would be insane to try to trace function graphs at these
> stages.
> 
> This patch makes the function graph tracer listening on power events, making
> it's tracing disabled for the current task (the one that performs the hibernation work)
> while suspend/resume to disk, making the tracing safe during hibernation.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Thanks, Frederic, I'll apply it and pass it on.

-- Steve

Re: [PATCH] tracing/function-graph-tracer: fix a regression while suspend to disk

[Pavel Machek]

> Impact: fix a crash while kernel image restore
> 
> When the function graph tracer is running and while suspend to disk, some racy
> and dangerous things happen against this tracer.
> 
> The current task will save its registers including the stack pointer which
> contains the return address hooked by the tracer. But the current task will
> continue to enter other functions after that to save the memory, and then
> it will store other return addresses, and finally loose the old depth which
> matches the return address saved in the old stack (during the registers saving).
> 
> So on image restore, the code will return to wrong addresses.
> And there are other things: on restore, the task will have it's "current"
> pointer overwritten during registers restoring....switching from one task to
> another... That would be insane to try to trace function graphs at these
> stages.
> 
> This patch makes the function graph tracer listening on power events, making
> it's tracing disabled for the current task (the one that performs the hibernation work)
> while suspend/resume to disk, making the tracing safe during hibernation.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Acked-by: Pavel Machek <pavel@suse.cz>

> ---
>  kernel/trace/ftrace.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 8c1c9c0..7e9a20b 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -17,6 +17,7 @@
>  #include <linux/clocksource.h>
>  #include <linux/kallsyms.h>
>  #include <linux/seq_file.h>
> +#include <linux/suspend.h>
>  #include <linux/debugfs.h>
>  #include <linux/hardirq.h>
>  #include <linux/kthread.h>
> @@ -1957,6 +1958,7 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  
>  static atomic_t ftrace_graph_active;
> +static struct notifier_block ftrace_suspend_notifier;
>  
>  int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
>  {
> @@ -2035,6 +2037,27 @@ static int start_graph_tracing(void)
>  	return ret;
>  }
>  
> +/*
> + * Hibernation protection.
> + * The state of the current task is too much unstable during
> + * suspend/restore to disk. We want to protect against that.
> + */
> +static int
> +ftrace_suspend_notifier_call(struct notifier_block *bl, unsigned long state,
> +							void *unused)
> +{
> +	switch (state) {
> +	case PM_HIBERNATION_PREPARE:
> +		pause_graph_tracing();
> +		break;
> +
> +	case PM_POST_HIBERNATION:
> +		unpause_graph_tracing();
> +		break;
> +	}
> +	return NOTIFY_DONE;
> +}
> +
>  int register_ftrace_graph(trace_func_graph_ret_t retfunc,
>  			trace_func_graph_ent_t entryfunc)
>  {
> @@ -2042,6 +2065,9 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
>  
>  	mutex_lock(&ftrace_sysctl_lock);
>  
> +	ftrace_suspend_notifier.notifier_call = ftrace_suspend_notifier_call;
> +	register_pm_notifier(&ftrace_suspend_notifier);
> +
>  	atomic_inc(&ftrace_graph_active);
>  	ret = start_graph_tracing();
>  	if (ret) {
> @@ -2067,6 +2093,7 @@ void unregister_ftrace_graph(void)
>  	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
>  	ftrace_graph_entry = ftrace_graph_entry_stub;
>  	ftrace_shutdown(FTRACE_STOP_FUNC_RET);
> +	unregister_pm_notifier(&ftrace_suspend_notifier);
>  
>  	mutex_unlock(&ftrace_sysctl_lock);
>  }

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html