Messages in this thread | | | Date | Wed, 3 Dec 2008 17:26:47 +0100 | From | "Frédéric Weisbecker" <> | Subject | Re: [PATCH 2/2] ftrace: fix race in function graph during fork |
| |
2008/12/3 Steven Rostedt <rostedt@goodmis.org>: > From: Steven Rostedt <srostedt@redhat.com> > > Impact: fix > > There is a nasy race in startup of a new process running the > function graph tracer. In fork.c: > > total_forks++; > spin_unlock(¤t->sighand->siglock); > write_unlock_irq(&tasklist_lock); > ftrace_graph_init_task(p); > proc_fork_connector(p); > cgroup_post_fork(p); > return p; > > The new task is free to run as soon as the tasklist_lock is released. > This is before the ftrace_graph_init_task. If the task does run > it will be using the same ret_stack and curr_ret_stack as the parent. > This will cause horrible crashes that are nasty to debug. > > This patch moves the ftrace_graph_init_task to just after the alloc_pid > code. This fixes the above race. > > Signed-off-by: Steven Rostedt <srostedt@redhat.com> > --- > kernel/fork.c | 9 ++++++--- > 1 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index afb376d..91cca76 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1142,6 +1142,8 @@ 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) > @@ -1150,7 +1152,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_pid; > + goto bad_fork_free_graph; > } > > p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL; > @@ -1243,7 +1245,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, > spin_unlock(¤t->sighand->siglock); > write_unlock_irq(&tasklist_lock); > retval = -ERESTARTNOINTR; > - goto bad_fork_free_pid; > + goto bad_fork_free_graph; > } > > if (clone_flags & CLONE_THREAD) { > @@ -1276,11 +1278,12 @@ static struct task_struct *copy_process(unsigned long clone_flags, > total_forks++; > spin_unlock(¤t->sighand->siglock); > write_unlock_irq(&tasklist_lock); > - ftrace_graph_init_task(p); > proc_fork_connector(p); > 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);
Very nice catch! Thanks.
| |