Re: [RFC][PATCH] tracing: Warn and notify if tracepoints are not loaded due to module taint

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 26 Feb 2014 00:39:01 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> ----- Original Message -----
> > From: "Steven Rostedt" <rostedt@goodmis.org>
> > To: "LKML" <linux-kernel@vger.kernel.org>
> > Cc: "Ingo Molnar" <mingo@kernel.org>, "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, "Rusty Russell"
> > <rusty@rustcorp.com.au>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>,
> > "Peter Zijlstra" <peterz@infradead.org>
> > Sent: Tuesday, February 25, 2014 7:15:05 PM
> > Subject: [RFC][PATCH] tracing: Warn and notify if tracepoints are not loaded due to module taint
> > 
> > [ Posting this as an RFC, but I plan on pushing it as soon as I finish
> > testing it ]
> > 
> > If a module is loaded that is tainted with anything but OOT or CRAP, then
> > it will not create the traceoint infrastructure for the module. There should
> 
> traceoint -> tracepoint

Oops, thanks.

> 
> > be a big warning when this happens instead of exiting silently.
> > 
> > Fixes: 97e1c18e8d17 "tracing: Kernel Tracepoints"
> > Cc: stable@vger.kernel.org
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> >  kernel/tracepoint.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index 29f2654..413bc06 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
> > @@ -641,7 +641,8 @@ static int tracepoint_module_coming(struct module *mod)
> >  	 * module headers (for forced load), to make sure we don't cause a crash.
> >  	 * Staging and out-of-tree GPL modules are fine.
> >  	 */
> > -	if (mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP)))
> > +	if (WARN_ONCE(mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP)),
> > +		      "Module is tainted, disabling tracepoints"))
> 
> Perhaps ever clearer with:
> 
>    "Module \"%s\" is tainted, disabling tracepoints", mod->name
> 
> ?

I originally had that with a simple WARN() instead of WARN_ONCE(), but
if you have that config which makes all modules not have sigs correct,
it spits out tens of these warnings and can cause more panic in users
than it deserves. I then switched it to WARN_ONCE(), and then thought,
that if it does it only once for the first module, it wont print the
warning again for the other affected modules. That means it may confuse
the user if they see a module had that warning, but the module they are
trying to trace isn't working either.

I then figured it would be good to remove the module name and just
state a general "Module is tainted, disabling tracepoints" and if the
user notices that the module isn't working, and then looks at their
dmesg, they'll see this message and just assume it was the module that
wasn't working.

Make sense?

-- Steve


> 
> Other than that, looks good to me!
>