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

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

[Steven Rostedt]

[ 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
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"))
 		return 0;
 	mutex_lock(&tracepoints_mutex);
 	tp_mod = kmalloc(sizeof(struct tp_module), GFP_KERNEL);
-- 
1.8.5.3

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

[Mathieu Desnoyers]

----- 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

> 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

?

Other than that, looks good to me!

Thanks,

Mathieu

>  		return 0;
>  	mutex_lock(&tracepoints_mutex);
>  	tp_mod = kmalloc(sizeof(struct tp_module), GFP_KERNEL);
> --
> 1.8.5.3
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

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

[Steven Rostedt]

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!
> 

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

[Peter Zijlstra]

On Tue, Feb 25, 2014 at 07:49:26PM -0500, Steven Rostedt wrote:
> > > -	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"))

> 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?

How about instead of a WARN, you use a normal KERN_ERR printk(). There's
no point to the entire WARN state dump, that's needlessly verbose.

When you have a normal error print you can have as many as are required
and put the mod name back in.

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

[Mathieu Desnoyers]

----- Original Message -----
> From: "Peter Zijlstra" <peterz@infradead.org>
> To: "Steven Rostedt" <rostedt@goodmis.org>
> Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, "LKML" <linux-kernel@vger.kernel.org>, "Ingo Molnar"
> <mingo@kernel.org>, "Rusty Russell" <rusty@rustcorp.com.au>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew
> Morton" <akpm@linux-foundation.org>
> Sent: Wednesday, February 26, 2014 3:59:26 AM
> Subject: Re: [RFC][PATCH] tracing: Warn and notify if tracepoints are not loaded due to module taint
> 
> On Tue, Feb 25, 2014 at 07:49:26PM -0500, Steven Rostedt wrote:
> > > > -	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"))
> 
> > 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?
> 
> How about instead of a WARN, you use a normal KERN_ERR printk(). There's
> no point to the entire WARN state dump, that's needlessly verbose.
> 
> When you have a normal error print you can have as many as are required
> and put the mod name back in.

The good old printk KERN_ERR is a very good idea. I agree that WARN() is
too verbose for our needs here.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

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

[Steven Rostedt]

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

> > How about instead of a WARN, you use a normal KERN_ERR printk(). There's
> > no point to the entire WARN state dump, that's needlessly verbose.
> > 
> > When you have a normal error print you can have as many as are required
> > and put the mod name back in.
> 
> The good old printk KERN_ERR is a very good idea. I agree that WARN() is
> too verbose for our needs here.

Actually, it's not so bad for the WARN() after my last patch to only
allocate (or even process tracepoints) if mod->num_tracepionts is
greater than zero. I didn't realize you were wasting memory for all
modules that were loaded.

My fear with the KERN_ERR is that it wont be noticeable enough. Where
as a stack dump is something that will catch people's attention.

And as Rusty has said, if you are loading a module that is forced, or
something strange, it is broken. The failure of loading the tracepoints
of a module is a bug if the module happens to have tracepoints.

After the MOD_SIG fix, any failure should be a big banner bug. Either
they are using a forced module with tracepoints that should not be
loaded. Or they have tracepoints is a non-GPL module (which is also a
big no-no).

-- Steve

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

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Peter Zijlstra" <peterz@infradead.org>, "LKML" <linux-kernel@vger.kernel.org>, "Ingo Molnar" <mingo@kernel.org>,
> "Rusty Russell" <rusty@rustcorp.com.au>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew Morton"
> <akpm@linux-foundation.org>
> Sent: Wednesday, February 26, 2014 11:15:42 AM
> Subject: Re: [RFC][PATCH] tracing: Warn and notify if tracepoints are not loaded due to module taint
> 
> On Wed, 26 Feb 2014 12:48:12 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> > > How about instead of a WARN, you use a normal KERN_ERR printk(). There's
> > > no point to the entire WARN state dump, that's needlessly verbose.
> > > 
> > > When you have a normal error print you can have as many as are required
> > > and put the mod name back in.
> > 
> > The good old printk KERN_ERR is a very good idea. I agree that WARN() is
> > too verbose for our needs here.
> 
> Actually, it's not so bad for the WARN() after my last patch to only
> allocate (or even process tracepoints) if mod->num_tracepionts is
> greater than zero. I didn't realize you were wasting memory for all
> modules that were loaded.
> 
> My fear with the KERN_ERR is that it wont be noticeable enough. Where
> as a stack dump is something that will catch people's attention.
> 
> And as Rusty has said, if you are loading a module that is forced, or
> something strange, it is broken. The failure of loading the tracepoints
> of a module is a bug if the module happens to have tracepoints.
> 
> After the MOD_SIG fix, any failure should be a big banner bug. Either
> they are using a forced module with tracepoints that should not be
> loaded. Or they have tracepoints is a non-GPL module (which is also a
> big no-no).

Agreed that after the skip for modules containing 0 tracepoints, it gets
much more specific. I like that.

So then a WARN_ON() that prints the specific module name involved would
be the way to go ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

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

[Steven Rostedt]

On Wed, 26 Feb 2014 17:24:47 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> ----- Original Message -----
> > From: "Steven Rostedt" <rostedt@goodmis.org>
> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> > Cc: "Peter Zijlstra" <peterz@infradead.org>, "LKML" <linux-kernel@vger.kernel.org>, "Ingo Molnar" <mingo@kernel.org>,
> > "Rusty Russell" <rusty@rustcorp.com.au>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew Morton"
> > <akpm@linux-foundation.org>
> > Sent: Wednesday, February 26, 2014 11:15:42 AM
> > Subject: Re: [RFC][PATCH] tracing: Warn and notify if tracepoints are not loaded due to module taint
> > 
> > On Wed, 26 Feb 2014 12:48:12 +0000 (UTC)
> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > 
> > > > How about instead of a WARN, you use a normal KERN_ERR printk(). There's
> > > > no point to the entire WARN state dump, that's needlessly verbose.
> > > > 
> > > > When you have a normal error print you can have as many as are required
> > > > and put the mod name back in.
> > > 
> > > The good old printk KERN_ERR is a very good idea. I agree that WARN() is
> > > too verbose for our needs here.
> > 
> > Actually, it's not so bad for the WARN() after my last patch to only
> > allocate (or even process tracepoints) if mod->num_tracepionts is
> > greater than zero. I didn't realize you were wasting memory for all
> > modules that were loaded.
> > 
> > My fear with the KERN_ERR is that it wont be noticeable enough. Where
> > as a stack dump is something that will catch people's attention.
> > 
> > And as Rusty has said, if you are loading a module that is forced, or
> > something strange, it is broken. The failure of loading the tracepoints
> > of a module is a bug if the module happens to have tracepoints.
> > 
> > After the MOD_SIG fix, any failure should be a big banner bug. Either
> > they are using a forced module with tracepoints that should not be
> > loaded. Or they have tracepoints is a non-GPL module (which is also a
> > big no-no).
> 
> Agreed that after the skip for modules containing 0 tracepoints, it gets
> much more specific. I like that.
> 
> So then a WARN_ON() that prints the specific module name involved would
> be the way to go ?


OK, I have a series of patches to fix a lot of these problems that I
will be posting soon. I'm fine with either a WARN() here (with module
name) or just a pr_err(). Which of theses do others think is the proper
answer?

Peter, Rusty, Andrew?

-- Steve