* [PATCH 0/2] [GIT PULL] tracing: Minor fixes
@ 2010-10-21 2:42 Steven Rostedt
2010-10-21 2:42 ` [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk() Steven Rostedt
2010-10-21 2:42 ` [PATCH 2/2] tracing: Do not limit the size of the number of CPU buffers Steven Rostedt
0 siblings, 2 replies; 9+ messages in thread
From: Steven Rostedt @ 2010-10-21 2:42 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker
Ingo,
Please pull the latest tip/perf/core tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/perf/core
Steven Rostedt (2):
tracing: Prevent unloadable modules from using trace_bprintk()
tracing: Do not limit the size of the number of CPU buffers
----
include/linux/kernel.h | 21 +++++++++++++++++++--
kernel/trace/trace.c | 8 ++------
kernel/trace/trace_printk.c | 2 ++
3 files changed, 23 insertions(+), 8 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()
2010-10-21 2:42 [PATCH 0/2] [GIT PULL] tracing: Minor fixes Steven Rostedt
@ 2010-10-21 2:42 ` Steven Rostedt
2010-10-21 3:42 ` Frederic Weisbecker
2010-10-21 8:16 ` Ingo Molnar
2010-10-21 2:42 ` [PATCH 2/2] tracing: Do not limit the size of the number of CPU buffers Steven Rostedt
1 sibling, 2 replies; 9+ messages in thread
From: Steven Rostedt @ 2010-10-21 2:42 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, Thomas Gleixner
[-- Attachment #1: 0001-tracing-Prevent-unloadable-modules-from-using-trace_.patch --]
[-- Type: text/plain, Size: 3497 bytes --]
From: Steven Rostedt <srostedt@redhat.com>
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@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
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)
{
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] tracing: Do not limit the size of the number of CPU buffers
2010-10-21 2:42 [PATCH 0/2] [GIT PULL] tracing: Minor fixes Steven Rostedt
2010-10-21 2:42 ` [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk() Steven Rostedt
@ 2010-10-21 2:42 ` Steven Rostedt
1 sibling, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2010-10-21 2:42 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker
[-- Attachment #1: 0002-tracing-Do-not-limit-the-size-of-the-number-of-CPU-b.patch --]
[-- Type: text/plain, Size: 1141 bytes --]
From: Steven Rostedt <srostedt@redhat.com>
The tracing per_cpu buffers were limited to 999 CPUs for a mear
savings in stack space of a char array. Up the array to 30 characters
which is more than enough to hold a 64 bit number.
Reported-by: Robin Holt <holt@sgi.com>
Suggested-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
kernel/trace/trace.c | 8 ++------
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 001bcd2..82d9b81 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3996,13 +3996,9 @@ static void tracing_init_debugfs_percpu(long cpu)
{
struct dentry *d_percpu = tracing_dentry_percpu();
struct dentry *d_cpu;
- /* strlen(cpu) + MAX(log10(cpu)) + '\0' */
- char cpu_dir[7];
+ char cpu_dir[30]; /* 30 characters should be more than enough */
- if (cpu > 999 || cpu < 0)
- return;
-
- sprintf(cpu_dir, "cpu%ld", cpu);
+ snprintf(cpu_dir, 30, "cpu%ld", cpu);
d_cpu = debugfs_create_dir(cpu_dir, d_percpu);
if (!d_cpu) {
pr_warning("Could not create debugfs '%s' entry\n", cpu_dir);
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()
2010-10-21 2:42 ` [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk() Steven Rostedt
@ 2010-10-21 3:42 ` Frederic Weisbecker
2010-10-21 3:47 ` Steven Rostedt
2010-10-21 8:16 ` Ingo Molnar
1 sibling, 1 reply; 9+ messages in thread
From: Frederic Weisbecker @ 2010-10-21 3:42 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Andrew Morton, Thomas Gleixner
On Wed, Oct 20, 2010 at 10:42:34PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
>
> 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@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> 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) && \
Did you mean CONFIG_MODULE may be?
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()
2010-10-21 3:42 ` Frederic Weisbecker
@ 2010-10-21 3:47 ` Steven Rostedt
2010-10-21 3:54 ` Frederic Weisbecker
0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2010-10-21 3:47 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Thomas Gleixner
On Thu, 2010-10-21 at 05:42 +0200, Frederic Weisbecker wrote:
> > +/*
> > + * 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) && \
>
>
>
> Did you mean CONFIG_MODULE may be?
Nope, then the a kernel that allows modules wont use it. I do mean
"MODULE", as in defined by the Makefile:
KBUILD_AFLAGS_MODULE := -DMODULE
KBUILD_CFLAGS_MODULE := -DMODULE
-- Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()
2010-10-21 3:47 ` Steven Rostedt
@ 2010-10-21 3:54 ` Frederic Weisbecker
0 siblings, 0 replies; 9+ messages in thread
From: Frederic Weisbecker @ 2010-10-21 3:54 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Andrew Morton, Thomas Gleixner
On Wed, Oct 20, 2010 at 11:47:56PM -0400, Steven Rostedt wrote:
> On Thu, 2010-10-21 at 05:42 +0200, Frederic Weisbecker wrote:
>
> > > +/*
> > > + * 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) && \
> >
> >
> >
> > Did you mean CONFIG_MODULE may be?
>
> Nope, then the a kernel that allows modules wont use it. I do mean
> "MODULE", as in defined by the Makefile:
>
> KBUILD_AFLAGS_MODULE := -DMODULE
> KBUILD_CFLAGS_MODULE := -DMODULE
Ah ok.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()
2010-10-21 2:42 ` [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk() Steven Rostedt
2010-10-21 3:42 ` Frederic Weisbecker
@ 2010-10-21 8:16 ` Ingo Molnar
2010-10-21 10:57 ` Steven Rostedt
1 sibling, 1 reply; 9+ messages in thread
From: Ingo Molnar @ 2010-10-21 8:16 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Andrew Morton, Frederic Weisbecker, Thomas Gleixner
* Steven Rostedt <rostedt@goodmis.org> wrote:
> From: Steven Rostedt <srostedt@redhat.com>
>
> 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@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> 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))
Erm. Ignore checkpatch when the solution is to mess up the code ...
> +# 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
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.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()
2010-10-21 8:16 ` Ingo Molnar
@ 2010-10-21 10:57 ` Steven Rostedt
2010-10-21 11:05 ` Ingo Molnar
0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2010-10-21 10:57 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, Andrew Morton, Frederic Weisbecker, Thomas Gleixner,
Rusty Russell, Linus Torvalds
On Thu, 2010-10-21 at 10:16 +0200, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > From: Steven Rostedt <srostedt@redhat.com>
> >
> > 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@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> > 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))
>
> Erm. Ignore checkpatch when the solution is to mess up the code ...
Heh, OK.
>
> > +# 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
>
> 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.
One thing that bothers me about this patch is the negative of
FORCE_TRACEPRINTK. I think it would be a nicer patch if I changed that
to a positive TRACE_BPRINTK_ALLOWED, then it would be:
+ if (__builtin_constant_p(fmt) && TRACE_BPRINTK_ALLOWED) {
+#if TRACE_BPRINTK_ALLOWED
EXPORT_SYMBOL_GPL(__trace_bprintk);
+#endif
The patch is still a bit ugly, but this change makes it a little nicer
to read.
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()
2010-10-21 10:57 ` Steven Rostedt
@ 2010-10-21 11:05 ` Ingo Molnar
0 siblings, 0 replies; 9+ messages in thread
From: Ingo Molnar @ 2010-10-21 11:05 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Andrew Morton, Frederic Weisbecker, Thomas Gleixner,
Rusty Russell, Linus Torvalds
* Steven Rostedt <rostedt@goodmis.org> 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@goodmis.org> -----
Date: Wed, 20 Oct 2010 22:42:34 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: linux-kernel@vger.kernel.org
Cc: Ingo Molnar <mingo@elte.hu>, Andrew Morton <akpm@linux-foundation.org>,
Frederic Weisbecker <fweisbec@gmail.com>,
Thomas Gleixner <tglx@linutronix.de>
Subject: [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk()
From: Steven Rostedt <srostedt@redhat.com>
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@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
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)
{
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-10-21 11:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-21 2:42 [PATCH 0/2] [GIT PULL] tracing: Minor fixes Steven Rostedt
2010-10-21 2:42 ` [PATCH 1/2] tracing: Prevent unloadable modules from using trace_bprintk() Steven Rostedt
2010-10-21 3:42 ` Frederic Weisbecker
2010-10-21 3:47 ` Steven Rostedt
2010-10-21 3:54 ` Frederic Weisbecker
2010-10-21 8:16 ` Ingo Molnar
2010-10-21 10:57 ` Steven Rostedt
2010-10-21 11:05 ` Ingo Molnar
2010-10-21 2:42 ` [PATCH 2/2] tracing: Do not limit the size of the number of CPU buffers Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox