Re: [PATCH 1/2] tracing: Prevent unloadable modules from usingtrace_bprintk()

[Ingo Molnar]

* Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> > > +#endif
> > 
> > Looks quite ugly all around. Cannot suggest anything better though straight away 
> > - so please Cc: it more widely and get an ack from the module folks: Rusty, 
> > Linus, akpm.
> 
> Just added them.

Below is the full patch again.

	Ingo

----- Forwarded message from Steven Rostedt <rostedt@xxxxxxxxxxx> -----

Date: Wed, 20 Oct 2010 22:42:34 -0400
From: Steven Rostedt <rostedt@xxxxxxxxxxx>
To: linux-kernel@xxxxxxxxxxxxxxx
Cc: Ingo Molnar <mingo@xxxxxxx>, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>,
	Frederic Weisbecker <fweisbec@xxxxxxxxx>,
	Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Subject: [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()

From: Steven Rostedt <srostedt@xxxxxxxxxx>

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: Frederic Weisbecker <fweisbec@xxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
---
 include/linux/kernel.h      |   21 +++++++++++++++++++--
 kernel/trace/trace_printk.c |    2 ++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2b0a35e..1003476 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -538,6 +538,23 @@ 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 FORCE_TRACEPRINTK 1
+#else
+# define FORCE_TRACEPRINTK 0
+#endif
+
 /**
  * trace_printk - printf formatting in the ftrace buffer
  * @fmt: the printf format for printing
@@ -558,14 +575,14 @@ do {									\
 #define trace_printk(fmt, args...)					\
 do {									\
 	__trace_printk_check_format(fmt, ##args);			\
-	if (__builtin_constant_p(fmt)) {				\
+	if (__builtin_constant_p(fmt) && !FORCE_TRACEPRINTK) {		\
 		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..c4a5db6 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 !FORCE_TRACEPRINTK
 EXPORT_SYMBOL_GPL(__trace_bprintk);
+#endif
 
 int __ftrace_vbprintk(unsigned long ip, const char *fmt, va_list ap)
  {

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/