Re: [PATCH] tracing: Fix infinite loop in ftrace_update_pid_func()

[Matt Fleming <matt@console-pimps.org>]

On Tue, Sep 29, 2009 at 11:22:47AM +0200, Frederic Weisbecker wrote:
> On Mon, Sep 28, 2009 at 04:43:01PM +0100, Matt Fleming wrote:
> > From: Matt Fleming <matthew.fleming@imgtec.com>
> > 
> > When CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST is enabled
> > __ftrace_trace_function contains the current trace function, not
> > ftrace_trace_function. In ftrace_update_pid_func() we currently
> > incorrectly assign the value of ftrace_trace_function to
> > __ftrace_trace_funcion before returning.
> > 
> > Without this patch it is possible to execute an infinite loop whereby
> > ftrace_test_stop_func() calls __ftrace_trace_function, which was
> > assigned ftrace_test_stop_func() in ftrace_update_pid_func().
> > 
> > Signed-off-by: Matt Fleming <matthew.fleming@imgtec.com>
> > ---
> >  kernel/trace/ftrace.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 25edd5c..d9ba6d9 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -225,7 +225,11 @@ static void ftrace_update_pid_func(void)
> >  	if (ftrace_trace_function == ftrace_stub)
> >  		return;
> >  
> > +#ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
> >  	func = ftrace_trace_function;
> > +#else
> > +	func = __ftrace_trace_function;
> > +#endif
> 
> 
> 
> I don't understand how it can do that infinite loop.
> It doesn't seem the following can happen:
> 
> func = ftrace_trace_function  //which is ftrace_test_stop_func
> 
> _ftrace_trace_function = func
> 

Here is the unpatched version of ftrace_update_pid_func() from
kernel/trace/ftrace.c

Before calling:
       ftrace_trace_function = ftrace_test_stop_func
       __ftrace_trace_Function = real tracing function

static void ftrace_update_pid_func(void)
{
	ftrace_func_t func;

	if (ftrace_trace_function == ftrace_stub)
		return;

	func = ftrace_trace_function;

	if (ftrace_pid_trace) {
		set_ftrace_pid_function(func);
		func = ftrace_pid_func;
	} else {
		if (func == ftrace_pid_func)
			func = ftrace_pid_function;
	}

#ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
	ftrace_trace_function = func;
#else
	__ftrace_trace_function = func;
#endif
}

After calling:
      ftrace_trace_function = ftrace_test_stop_func
      __ftrace_trace_function = ftrace_test_stop_func


Then look what happens next time ftrace_test_stop_func() is called via
ftrace_call,

#ifndef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
/*
 * For those archs that do not test ftrace_trace_stop in their
 * mcount call site, we need to do it from C.
 */
static void ftrace_test_stop_func(unsigned long ip, unsigned long parent_ip)
{
	if (function_trace_stop)
		return;

	__ftrace_trace_function(ip, parent_ip);
}
#endif

Here we're going to recurse infinitely because,
     __ftrace_trace_function = ftrace_test_stop_func