[PATCH 0/2] [GIT PULL][urgent] function-graph: allocation and freeing of ret_stack

[PATCH 0/2] [GIT PULL][urgent] function-graph: allocation and freeing of ret_stack

[Steven Rostedt]

Ingo,

Here's a couple of more fixes.

Please pull the latest tip/tracing/urgent-1 tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/tracing/urgent-1


Steven Rostedt (2):
      function-graph: move initialization of new tasks up in fork
      function-graph: always initialize task ret_stack

----
 kernel/fork.c         |   10 ++++------
 kernel/trace/ftrace.c |    6 ++++--
 2 files changed, 8 insertions(+), 8 deletions(-)
-- 

[PATCH 1/2] function-graph: move initialization of new tasks up in fork

[Steven Rostedt]

[-- Attachment #1: 0001-function-graph-move-initialization-of-new-tasks-up-i.patch --]
[-- Type: text/plain, Size: 2386 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

When the function graph tracer is enabled, all new tasks must allocate
a ret_stack to place the return address of functions. This is because
the function graph tracer will replace the real return address with a
call to the tracing of the exit function.

This initialization happens in fork, but it happens too late. If fork
fails, then it will call free_task and that calls the freeing of this
ret_stack. But before initialization happens, the new (failed) task
points to its parents ret_stack. If a fork failure happens during
the function trace, it would be catastrophic for the parent.

Also, there's no need to call ftrace_graph_exit_task from fork, since
it is called by free_task which fork calls on failure.

[ Impact: prevent crash during failed fork running function graph tracer ]

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

diff --git a/kernel/fork.c b/kernel/fork.c
index b9e2edd..c4b1e35 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -982,6 +982,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	if (!p)
 		goto fork_out;
 
+	ftrace_graph_init_task(p);
+
 	rt_mutex_init_task(p);
 
 #ifdef CONFIG_PROVE_LOCKING
@@ -1131,8 +1133,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		}
 	}
 
-	ftrace_graph_init_task(p);
-
 	p->pid = pid_nr(pid);
 	p->tgid = p->pid;
 	if (clone_flags & CLONE_THREAD)
@@ -1141,7 +1141,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	if (current->nsproxy != p->nsproxy) {
 		retval = ns_cgroup_clone(p, pid);
 		if (retval)
-			goto bad_fork_free_graph;
+			goto bad_fork_free_pid;
 	}
 
 	p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL;
@@ -1233,7 +1233,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		spin_unlock(&current->sighand->siglock);
 		write_unlock_irq(&tasklist_lock);
 		retval = -ERESTARTNOINTR;
-		goto bad_fork_free_graph;
+		goto bad_fork_free_pid;
 	}
 
 	if (clone_flags & CLONE_THREAD) {
@@ -1268,8 +1268,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	cgroup_post_fork(p);
 	return p;
 
-bad_fork_free_graph:
-	ftrace_graph_exit_task(p);
 bad_fork_free_pid:
 	if (pid != &init_struct_pid)
 		free_pid(pid);
-- 
1.6.3.1

-- 

Re: [PATCH 1/2] function-graph: move initialization of new tasks up in fork

[Frederic Weisbecker]

On Tue, Jun 02, 2009 at 06:02:13PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> When the function graph tracer is enabled, all new tasks must allocate
> a ret_stack to place the return address of functions. This is because
> the function graph tracer will replace the real return address with a
> call to the tracing of the exit function.
> 
> This initialization happens in fork, but it happens too late. If fork
> fails, then it will call free_task and that calls the freeing of this
> ret_stack. But before initialization happens, the new (failed) task
> points to its parents ret_stack. If a fork failure happens during
> the function trace, it would be catastrophic for the parent.
> 
> Also, there's no need to call ftrace_graph_exit_task from fork, since
> it is called by free_task which fork calls on failure.
> 
> [ Impact: prevent crash during failed fork running function graph tracer ]
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


Ah, thanks a lot. It was on my TODO list.
Oleg Nesterov reported me that problem...

Frederic.


> ---
>  kernel/fork.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b9e2edd..c4b1e35 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -982,6 +982,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	if (!p)
>  		goto fork_out;
>  
> +	ftrace_graph_init_task(p);
> +
>  	rt_mutex_init_task(p);
>  
>  #ifdef CONFIG_PROVE_LOCKING
> @@ -1131,8 +1133,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  		}
>  	}
>  
> -	ftrace_graph_init_task(p);
> -
>  	p->pid = pid_nr(pid);
>  	p->tgid = p->pid;
>  	if (clone_flags & CLONE_THREAD)
> @@ -1141,7 +1141,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	if (current->nsproxy != p->nsproxy) {
>  		retval = ns_cgroup_clone(p, pid);
>  		if (retval)
> -			goto bad_fork_free_graph;
> +			goto bad_fork_free_pid;
>  	}
>  
>  	p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL;
> @@ -1233,7 +1233,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  		spin_unlock(&current->sighand->siglock);
>  		write_unlock_irq(&tasklist_lock);
>  		retval = -ERESTARTNOINTR;
> -		goto bad_fork_free_graph;
> +		goto bad_fork_free_pid;
>  	}
>  
>  	if (clone_flags & CLONE_THREAD) {
> @@ -1268,8 +1268,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	cgroup_post_fork(p);
>  	return p;
>  
> -bad_fork_free_graph:
> -	ftrace_graph_exit_task(p);
>  bad_fork_free_pid:
>  	if (pid != &init_struct_pid)
>  		free_pid(pid);
> -- 
> 1.6.3.1
> 
> -- 

[PATCH 2/2] function-graph: always initialize task ret_stack

[Steven Rostedt]

[-- Attachment #1: 0002-function-graph-always-initialize-task-ret_stack.patch --]
[-- Type: text/plain, Size: 1341 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

On creating a new task while running the function graph tracer, if
we fail to allocate the ret_stack, and then fail the fork, the
code will free the parent ret_stack. This is because the child
duplicated the parent and currently points to the parent's ret_stack.

This patch always initializes the task's ret_stack to NULL.

[ Impact: prevent crash of parent on low memory during fork ]

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

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 1664d3f..bb081f3 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2738,6 +2738,9 @@ void unregister_ftrace_graph(void)
 /* Allocate a return stack for newly created task */
 void ftrace_graph_init_task(struct task_struct *t)
 {
+	/* Make sure we do not use the parent ret_stack */
+	t->ret_stack = NULL;
+
 	if (atomic_read(&ftrace_graph_active)) {
 		struct ftrace_ret_stack *ret_stack;
 
@@ -2753,8 +2756,7 @@ void ftrace_graph_init_task(struct task_struct *t)
 		/* make curr_ret_stack visable before we add the ret_stack */
 		smp_wmb();
 		t->ret_stack = ret_stack;
-	} else
-		t->ret_stack = NULL;
+	}
 }
 
 void ftrace_graph_exit_task(struct task_struct *t)
-- 
1.6.3.1

-- 

Re: [PATCH 0/2] [GIT PULL][urgent] function-graph: allocation and freeing of ret_stack

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Ingo,
> 
> Here's a couple of more fixes.
> 
> Please pull the latest tip/tracing/urgent-1 tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/tracing/urgent-1
> 
> 
> Steven Rostedt (2):
>       function-graph: move initialization of new tasks up in fork
>       function-graph: always initialize task ret_stack
> 
> ----
>  kernel/fork.c         |   10 ++++------
>  kernel/trace/ftrace.c |    6 ++++--
>  2 files changed, 8 insertions(+), 8 deletions(-)

Pulled, thanks Steve!

( Note, we need to keep these two commits in mind and forward them 
  to stable@kernel.org in a few days - as they have no Cc:
  stable@kernel.org tags. )

	Ingo