[PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Steven Rostedt]

Ingo,

This is based off of my core-2 branch. I'm moved this patch after that
so if anyone has any objections, I can change this patch without holding
off the previous one.

Note, I made the TRACE_BPRINTK_ALLOWED change since it looks better.

Thanks,

-- Steve

Please pull the latest tip/perf/core-3 tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/perf/core-3


Steven Rostedt (1):
      tracing: Prevent unloadable modules from using trace_bprintk()

----
 include/linux/kernel.h      |   20 ++++++++++++++++++--
 kernel/trace/trace_printk.c |    2 ++
 2 files changed, 20 insertions(+), 2 deletions(-)
---------------------------
commit c3b87c579e5df72a59fe97d77d4c3791dc8154ec
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Wed Oct 20 20:50:00 2010 -0400

    tracing: Prevent unloadable modules from using trace_bprintk()
    
    While debugging a module, I found that unloading the module and
    then reading the ring buffer can cause strange side effects, including
    a kernel crash.
    
    This is due to the trace_bprintk(). The trace_bprintk() is a faster
    version of trace_printk(). The difference is that trace_bprintk()
    only copies the arguments and a pointer to the format string into
    the ring buffer.
    
    If a module uses this function and is unloaded, the pointer back to
    the format string in the module is still around. If the trace file
    is read, then the pointer is referenced and this can cause a kernel
    oops.
    
    The simple solution is to not let modules use trace_bprintk() and
    instead it will use the slower version of this.
    
    When talking with Frederic Weisbecker about it, he suggested not to
    punish modules that can not be unloaded since they do not have
    this side effect. Modules that can not be unloaded can still use
    trace_bprintk(). We added a check for MODVERSIONS to be set to make
    sure that the module and kernel have the same options. If you
    run without MODVERSIONS set, and you load a module that was compiled
    differently, then that's just your tough luck.
    
    Cc: Rusty Russell <rusty@rustcorp.com.au>
    Cc: Linus Torvalds <torvalds@linux-foundation.org>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Thomas Gleixner <tglx@linutronix.de>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2b0a35e..9c683f3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -538,6 +538,22 @@ do {									\
 		____trace_printk_check_format(fmt, ##args);		\
 } while (0)
 
+/*
+ * Module code must not use trace_bprintk, because if it is unloaded
+ * then we leave a pointer back to the module code inside
+ * the ring buffer, and then reading the ring buffer may cause a bug.
+ *
+ * We do allow for modules to use it if the kernel does not allow
+ * unloading of modules, and MODVERSIONS is set (to make sure kernel
+ * and module are the same). If you load modules without MODVERSIONS
+ * set, then you deserve what you get.
+ */
+#if defined(MODULE) && (defined(CONFIG_MODULE_UNLOAD) || !defined(CONFIG_MODVERSIONS))
+# define TRACE_BPRINTK_ALLOWED 0
+#else
+# define TRACE_BPRINTK_ALLOWED 1
+#endif
+
 /**
  * trace_printk - printf formatting in the ftrace buffer
  * @fmt: the printf format for printing
@@ -558,14 +574,14 @@ do {									\
 #define trace_printk(fmt, args...)					\
 do {									\
 	__trace_printk_check_format(fmt, ##args);			\
-	if (__builtin_constant_p(fmt)) {				\
+	if (__builtin_constant_p(fmt) && TRACE_BPRINTK_ALLOWED) {	\
 		static const char *trace_printk_fmt			\
 		  __attribute__((section("__trace_printk_fmt"))) =	\
 			__builtin_constant_p(fmt) ? fmt : NULL;		\
 									\
 		__trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args);	\
 	} else								\
-		__trace_printk(_THIS_IP_, fmt, ##args);		\
+		__trace_printk(_THIS_IP_, fmt, ##args);			\
 } while (0)
 
 extern int
diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index 2547d88..230bbd9 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -115,7 +115,9 @@ int __trace_bprintk(unsigned long ip, const char *fmt, ...)
 	va_end(ap);
 	return ret;
 }
+#if TRACE_BPRINTK_ALLOWED
 EXPORT_SYMBOL_GPL(__trace_bprintk);
+#endif
 
 int __ftrace_vbprintk(unsigned long ip, const char *fmt, va_list ap)
  {

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Rusty Russell]

On Fri, 22 Oct 2010 12:15:42 am Steven Rostedt wrote:
> 
> Ingo,
> 
> This is based off of my core-2 branch. I'm moved this patch after that
> so if anyone has any objections, I can change this patch without holding
> off the previous one.

I think disabling use in modules is lazy, and allowing it for !MODVERSIONS
is just weird.

Can't you detect this on module unload and fix it up?  Or delay freeing the
module until the trace ring is emptied?

Cheers,
Rusty.

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Steven Rostedt]

On Fri, 2010-10-22 at 08:05 +1030, Rusty Russell wrote:
> On Fri, 22 Oct 2010 12:15:42 am Steven Rostedt wrote:
> > 
> > Ingo,
> > 
> > This is based off of my core-2 branch. I'm moved this patch after that
> > so if anyone has any objections, I can change this patch without holding
> > off the previous one.
> 
> I think disabling use in modules is lazy,

and safer.

It's not a big deal if a module does not use it. What we have is
"trace_printk()" and if the format passed to it is static, that turns
into trace_bprintk(). If the format is a variable, then you are stuck
with trace_printk() that just does the sprintf() operation in the trace.
Which this patch makes all modules do.

With trace_bprintk() the pointer to the format and arguments are put
into the ring buffer for later processing.


>  and allowing it for !MODVERSIONS
> is just weird.

that was me being weirdly paranoid ;-)

> 
> Can't you detect this on module unload and fix it up?  Or delay freeing the
> module until the trace ring is emptied?

One possibility is to magically make all string formats used in
trace_printk into its own section, and keep it allocated until the ring
buffer is empty. Or, we can just do that with the module's entire string
section, since we know whether or not that module has a trace_printk in
it or not.

-- Steve

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Rusty Russell]

On Fri, 22 Oct 2010 09:04:23 am Steven Rostedt wrote:
> On Fri, 2010-10-22 at 08:05 +1030, Rusty Russell wrote:
> > On Fri, 22 Oct 2010 12:15:42 am Steven Rostedt wrote:
> > > 
> > > Ingo,
> > > 
> > > This is based off of my core-2 branch. I'm moved this patch after that
> > > so if anyone has any objections, I can change this patch without holding
> > > off the previous one.
> > 
> > I think disabling use in modules is lazy,
> 
> and safer.

Well then just delete trace_bprintk altogether.  Even safer!

> > Can't you detect this on module unload and fix it up?  Or delay freeing the
> > module until the trace ring is emptied?
> 
> One possibility is to magically make all string formats used in
> trace_printk into its own section, and keep it allocated until the ring
> buffer is empty. Or, we can just do that with the module's entire string
> section, since we know whether or not that module has a trace_printk in
> it or not.

Exactly.  Set a flag in the module if it resolves trace_printk, and defer freeing
the module in that case.  This shouldn't be that hard...

Rusty.

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Steven Rostedt]

On Fri, 2010-10-22 at 14:13 +1030, Rusty Russell wrote:

> > > 
> > > I think disabling use in modules is lazy,
> > 
> > and safer.
> 
> Well then just delete trace_bprintk altogether.  Even safer!

Reminds me of the argument against lowering the speed limit to 55. "it's
safer"... "Then lower it to zero, even safer!"


> 
> > > Can't you detect this on module unload and fix it up?  Or delay freeing the
> > > module until the trace ring is emptied?
> > 
> > One possibility is to magically make all string formats used in
> > trace_printk into its own section, and keep it allocated until the ring
> > buffer is empty. Or, we can just do that with the module's entire string
> > section, since we know whether or not that module has a trace_printk in
> > it or not.
> 
> Exactly.  Set a flag in the module if it resolves trace_printk, and defer freeing
> the module in that case.  This shouldn't be that hard...

Here's my worry.

1) Some module with tracepoints is loaded at boot up.
2) The user does tracing and forgets about it (ring buffer filled)
3) Unloads module (don't free)
4) loads module with trace points
5) unloads module (don't free)
etc, etc

memory leak.

Thus this is not that trivial. We probably need to have a way to lock a
module when its tracepoint is activated, and only unlock it when the
ring buffer is emptied.

Do we want to prevent the module from being unloaded while the ring
buffer is full (after that module has been traced?), or do we let the
module be unloaded, but just prevent this one section from being freed?

-- Steve

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Rusty Russell]

On Fri, 22 Oct 2010 02:28:38 pm Steven Rostedt wrote:
> On Fri, 2010-10-22 at 14:13 +1030, Rusty Russell wrote:
> 
> > > > 
> > > > I think disabling use in modules is lazy,
> > > 
> > > and safer.
> > 
> > Well then just delete trace_bprintk altogether.  Even safer!
> 
> Reminds me of the argument against lowering the speed limit to 55. "it's
> safer"... "Then lower it to zero, even safer!"
> 
> 
> > 
> > > > Can't you detect this on module unload and fix it up?  Or delay freeing the
> > > > module until the trace ring is emptied?
> > > 
> > > One possibility is to magically make all string formats used in
> > > trace_printk into its own section, and keep it allocated until the ring
> > > buffer is empty. Or, we can just do that with the module's entire string
> > > section, since we know whether or not that module has a trace_printk in
> > > it or not.
> > 
> > Exactly.  Set a flag in the module if it resolves trace_printk, and defer freeing
> > the module in that case.  This shouldn't be that hard...
> 
> Here's my worry.
> 
> 1) Some module with tracepoints is loaded at boot up.
> 2) The user does tracing and forgets about it (ring buffer filled)
> 3) Unloads module (don't free)
> 4) loads module with trace points
> 5) unloads module (don't free)
> etc, etc
> 
> memory leak.

Sure.  Then mark the rb count or something in the module at init time,
then compare before deciding too dangerous to free.

> Thus this is not that trivial. We probably need to have a way to lock a
> module when its tracepoint is activated, and only unlock it when the
> ring buffer is emptied.

How about the intuitive and completely obvious thing?  When a tp activated,
use the module.  When deactivated, unuse it?

> Do we want to prevent the module from being unloaded while the ring
> buffer is full (after that module has been traced?), or do we let the
> module be unloaded, but just prevent this one section from being freed?

I was thinking the latter, basically defer the module_free() call.

Cheers,
Rusty.

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Li Zefan]

>>>>> Can't you detect this on module unload and fix it up?  Or delay freeing the
>>>>> module until the trace ring is emptied?
>>>> One possibility is to magically make all string formats used in
>>>> trace_printk into its own section, and keep it allocated until the ring
>>>> buffer is empty. Or, we can just do that with the module's entire string
>>>> section, since we know whether or not that module has a trace_printk in
>>>> it or not.
>>> Exactly.  Set a flag in the module if it resolves trace_printk, and defer freeing
>>> the module in that case.  This shouldn't be that hard...
>> Here's my worry.
>>
>> 1) Some module with tracepoints is loaded at boot up.
>> 2) The user does tracing and forgets about it (ring buffer filled)
>> 3) Unloads module (don't free)
>> 4) loads module with trace points
>> 5) unloads module (don't free)
>> etc, etc
>>
>> memory leak.
> 
> Sure.  Then mark the rb count or something in the module at init time,
> then compare before deciding too dangerous to free.
> 
>> Thus this is not that trivial. We probably need to have a way to lock a
>> module when its tracepoint is activated, and only unlock it when the
>> ring buffer is emptied.
> 
> How about the intuitive and completely obvious thing?  When a tp activated,
> use the module.  When deactivated, unuse it?
> 

Do you mean inc module refcnt when a tp is activated, and dec the refcnt
if deactivated? That's what we already have.

In fact tracepoint is free from this bug, because we'll empty the ring
buffer if the unloading module has tracepoints in it.

So for trace_bprintk, why can't we do the same thing? If a module has
trace_bprintk calls in it, just empty the ring buffer when unloading
module.

And that's as simple as something like this:

diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index 2547d88..103987f 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -80,6 +80,13 @@ static int module_trace_bprintk_format_notify(struct notifier_block *self,
 
 		if (val == MODULE_STATE_COMING)
 			hold_module_trace_bprintk_format(start, end);
+		else if (val == MODULE_STATE_GOING) {
+			/*
+			 * It is safest to reset the ring buffer if the
+			 * module being unloaded uses trace_bprintk.
+			 */
+			tracing_reset_current_online_cpus();
+		}
 	}
 	return 0;
 }

>> Do we want to prevent the module from being unloaded while the ring
>> buffer is full (after that module has been traced?), or do we let the
>> module be unloaded, but just prevent this one section from being freed?
> 
> I was thinking the latter, basically defer the module_free() call.
> 

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Steven Rostedt]

On Fri, 2010-10-22 at 13:30 +0800, Li Zefan wrote:

> In fact tracepoint is free from this bug, because we'll empty the ring
> buffer if the unloading module has tracepoints in it.

Hehe, and I should know, I wrote that code :-)


> 
> So for trace_bprintk, why can't we do the same thing? If a module has
> trace_bprintk calls in it, just empty the ring buffer when unloading
> module.
> 
> And that's as simple as something like this:
> 
> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
> index 2547d88..103987f 100644
> --- a/kernel/trace/trace_printk.c
> +++ b/kernel/trace/trace_printk.c
> @@ -80,6 +80,13 @@ static int module_trace_bprintk_format_notify(struct notifier_block *self,
>  
>  		if (val == MODULE_STATE_COMING)
>  			hold_module_trace_bprintk_format(start, end);
> +		else if (val == MODULE_STATE_GOING) {
> +			/*
> +			 * It is safest to reset the ring buffer if the
> +			 * module being unloaded uses trace_bprintk.
> +			 */
> +			tracing_reset_current_online_cpus();
> +		}
>  	}
>  	return 0;
>  }

This could definitely work.

But this prevents developers using trace_printk() in their exit() code.
We should probably add a trace_mod_printk() or something that just
forces the slow version that does not require flushing the ring buffer
on removal. As long as there's not a single trace_printk() in the
module, and it only uses the alternative, then this should work.

-- Steve

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Li Zefan]

Steven Rostedt wrote:
> On Fri, 2010-10-22 at 13:30 +0800, Li Zefan wrote:
> 
>> In fact tracepoint is free from this bug, because we'll empty the ring
>> buffer if the unloading module has tracepoints in it.
> 
> Hehe, and I should know, I wrote that code :-)
> 

I reported that bug. ;)

> 
>> So for trace_bprintk, why can't we do the same thing? If a module has
>> trace_bprintk calls in it, just empty the ring buffer when unloading
>> module.
>>
>> And that's as simple as something like this:
>>
>> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
>> index 2547d88..103987f 100644
>> --- a/kernel/trace/trace_printk.c
>> +++ b/kernel/trace/trace_printk.c
>> @@ -80,6 +80,13 @@ static int module_trace_bprintk_format_notify(struct notifier_block *self,
>>  
>>  		if (val == MODULE_STATE_COMING)
>>  			hold_module_trace_bprintk_format(start, end);
>> +		else if (val == MODULE_STATE_GOING) {
>> +			/*
>> +			 * It is safest to reset the ring buffer if the
>> +			 * module being unloaded uses trace_bprintk.
>> +			 */
>> +			tracing_reset_current_online_cpus();
>> +		}
>>  	}
>>  	return 0;
>>  }
> 
> This could definitely work.
> 
> But this prevents developers using trace_printk() in their exit() code.
> We should probably add a trace_mod_printk() or something that just
> forces the slow version that does not require flushing the ring buffer
> on removal. As long as there's not a single trace_printk() in the
> module, and it only uses the alternative, then this should work.
> 

Agreed. Add this trace_mod_printk() and add some comments to
explain why and when use it.

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Ingo Molnar]

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

> 
> Ingo,
> 
> This is based off of my core-2 branch. I'm moved this patch after that
> so if anyone has any objections, I can change this patch without holding
> off the previous one.
> 
> Note, I made the TRACE_BPRINTK_ALLOWED change since it looks better.

Hm, it still looks rather fundamentally ugly, messy and assymetric. Please work out 
some better solution with the module folks. If there's really no sane solution then 
we better remove this 'speedup' API, it's just not worth it.

Thanks,

	Ingo

Re: [PATCH v2][GIT PULL] tracing: Prevent unloadable modules from using trace_bprintk()

[Steven Rostedt]

On Fri, 2010-10-22 at 10:05 +0200, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > Ingo,
> > 
> > This is based off of my core-2 branch. I'm moved this patch after that
> > so if anyone has any objections, I can change this patch without holding
> > off the previous one.
> > 
> > Note, I made the TRACE_BPRINTK_ALLOWED change since it looks better.
> 
> Hm, it still looks rather fundamentally ugly, messy and assymetric. Please work out 
> some better solution with the module folks. If there's really no sane solution then 
> we better remove this 'speedup' API, it's just not worth it.

If something doesn't work, then I say we just prevent all modules from
using it. Why punish kernel code. The API is already dynamic, since it
only uses the speed up if the format is static. We could just add no
modules too.

But that said, I think we do have a solution that Li Zefan pointed out
(and something I'm ashamed about not coming up with, since it was indeed
the obvious solution).

-- Steve