[PATCH 2/2] module: Re-enable dynamic debugging for GPL-compatible OOT modules

[PATCH 2/2] module: Re-enable dynamic debugging for GPL-compatible OOT modules

[Ben Hutchings]

Dynamic debugging was enabled for GPL-compatible out-of-tree modules
until my addition of TAINT_OOT_MODULE.  It should continue to be
enabled now.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 kernel/module.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index dab585e..448fd77 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2881,7 +2881,7 @@ static struct module *load_module(void __user *umod,
 	}
 
 	/* This has to be done once we're sure module name is unique. */
-	if (!mod->taints || mod->taints == (1U<<TAINT_CRAP))
+	if (!(mod->taints & ~(1U << TAINT_CRAP | 1U << TAINT_OOT_MODULE)))
 		dynamic_debug_setup(info.debug, info.num_debug);
 
 	/* Find duplicate symbols */
@@ -2918,7 +2918,7 @@ static struct module *load_module(void __user *umod,
 	module_bug_cleanup(mod);
 
  ddebug:
-	if (!mod->taints || mod->taints == (1U<<TAINT_CRAP))
+	if (!(mod->taints & ~(1U << TAINT_CRAP | 1U << TAINT_OOT_MODULE)))
 		dynamic_debug_remove(info.debug);
  unlock:
 	mutex_unlock(&module_mutex);
-- 
1.7.7

Re: [PATCH 2/2] module: Re-enable dynamic debugging for GPL-compatible OOT modules

[Rusty Russell]

On Fri, 28 Oct 2011 04:38:14 +0100, Ben Hutchings <ben@decadent.org.uk> wrote:
> Dynamic debugging was enabled for GPL-compatible out-of-tree modules
> until my addition of TAINT_OOT_MODULE.  It should continue to be
> enabled now.

Please just remove the test entirely.

AFAICT there's nothing unique to dynamic debug which means it should
avoid taint.  If it oopses, we'll learn all about tainting in the oops
message.

> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  kernel/module.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index dab585e..448fd77 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2881,7 +2881,7 @@ static struct module *load_module(void __user *umod,
>  	}
>  
>  	/* This has to be done once we're sure module name is unique. */
> -	if (!mod->taints || mod->taints == (1U<<TAINT_CRAP))
> +	if (!(mod->taints & ~(1U << TAINT_CRAP | 1U << TAINT_OOT_MODULE)))
>  		dynamic_debug_setup(info.debug, info.num_debug);
>  
>  	/* Find duplicate symbols */
> @@ -2918,7 +2918,7 @@ static struct module *load_module(void __user *umod,
>  	module_bug_cleanup(mod);
>  
>   ddebug:
> -	if (!mod->taints || mod->taints == (1U<<TAINT_CRAP))
> +	if (!(mod->taints & ~(1U << TAINT_CRAP | 1U << TAINT_OOT_MODULE)))
>  		dynamic_debug_remove(info.debug);
>   unlock:
>  	mutex_unlock(&module_mutex);
> -- 
> 1.7.7
> 
> 

Re: [PATCH 2/2] module: Re-enable dynamic debugging for GPL-compatible OOT modules

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]

On Mon, 2011-10-31 at 12:29 +1030, Rusty Russell wrote:
> On Fri, 28 Oct 2011 04:38:14 +0100, Ben Hutchings <ben@decadent.org.uk> wrote:
> > Dynamic debugging was enabled for GPL-compatible out-of-tree modules
> > until my addition of TAINT_OOT_MODULE.  It should continue to be
> > enabled now.
> 
> Please just remove the test entirely.
> 
> AFAICT there's nothing unique to dynamic debug which means it should
> avoid taint.  If it oopses, we'll learn all about tainting in the oops
> message.

It looks like the dynamic debug facility is not meant to be available to
proprietary modules.

Ben.

> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > ---
> >  kernel/module.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index dab585e..448fd77 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2881,7 +2881,7 @@ static struct module *load_module(void __user *umod,
> >  	}
> >  
> >  	/* This has to be done once we're sure module name is unique. */
> > -	if (!mod->taints || mod->taints == (1U<<TAINT_CRAP))
> > +	if (!(mod->taints & ~(1U << TAINT_CRAP | 1U << TAINT_OOT_MODULE)))
> >  		dynamic_debug_setup(info.debug, info.num_debug);
> >  
> >  	/* Find duplicate symbols */
> > @@ -2918,7 +2918,7 @@ static struct module *load_module(void __user *umod,
> >  	module_bug_cleanup(mod);
> >  
> >   ddebug:
> > -	if (!mod->taints || mod->taints == (1U<<TAINT_CRAP))
> > +	if (!(mod->taints & ~(1U << TAINT_CRAP | 1U << TAINT_OOT_MODULE)))
> >  		dynamic_debug_remove(info.debug);
> >   unlock:
> >  	mutex_unlock(&module_mutex);
> > -- 
> > 1.7.7
> > 
> > 
> 

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] module: Re-enable dynamic debugging for GPL-compatible OOT modules

[Rusty Russell]

On Mon, 31 Oct 2011 13:44:17 +0000, Ben Hutchings <ben@decadent.org.uk> wrote:
Non-text part: multipart/signed
> On Mon, 2011-10-31 at 12:29 +1030, Rusty Russell wrote:
> > On Fri, 28 Oct 2011 04:38:14 +0100, Ben Hutchings <ben@decadent.org.uk> wrote:
> > > Dynamic debugging was enabled for GPL-compatible out-of-tree modules
> > > until my addition of TAINT_OOT_MODULE.  It should continue to be
> > > enabled now.
> > 
> > Please just remove the test entirely.
> > 
> > AFAICT there's nothing unique to dynamic debug which means it should
> > avoid taint.  If it oopses, we'll learn all about tainting in the oops
> > message.
> 
> It looks like the dynamic debug facility is not meant to be available to
> proprietary modules.

That was my guess too, but Mathieu (the author) said it was about
malformed modules:

On Wed, 26 Oct 2011 02:15:21 -0400:
> This check for tainted modules was first introduced with markers, and
> then used by tracepoints, and then also by dynamic debug. The rationale
> for this check was mainly to ensure that the marker/tracepoint code
> would not trigger a crash when loading a module with incompatible module
> header, originally compiled for an older kernel, into a newer kernel.
> This problem would happen even if the said module does not contain any
> marker/tracepoint, because we happen to try to use fields that are
> non-existent in the module header.

This is pretty bogus: since they forced the module in the first place,
they can handle the explosion.

So we should drop it altogether.

Thanks,
Rusty.

[PATCH] module: Enable dynamic debugging regardless of taint

[Ben Hutchings]

Dynamic debugging is currently disabled for tainted modules, except
for TAINT_CRAP.  This prevents use of dynamic debugging for
out-of-tree modules now that they are also tainted.

This condition was apparently intended to avoid a crash if a force-
loaded module has an incompatible definition of dynamic debug
structures.  However, a administrator that forces us to load a module
is claiming that it *is* compatible even though it fails our version
checks.  If they are mistaken, there are any number of ways the module
could crash the system.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 kernel/module.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index dab585e..ef8cb70 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2881,8 +2881,7 @@ static struct module *load_module(void __user *umod,
 	}
 
 	/* This has to be done once we're sure module name is unique. */
-	if (!mod->taints || mod->taints == (1U<<TAINT_CRAP))
-		dynamic_debug_setup(info.debug, info.num_debug);
+	dynamic_debug_setup(info.debug, info.num_debug);
 
 	/* Find duplicate symbols */
 	err = verify_export_symbols(mod);
@@ -2918,8 +2917,7 @@ static struct module *load_module(void __user *umod,
 	module_bug_cleanup(mod);
 
  ddebug:
-	if (!mod->taints || mod->taints == (1U<<TAINT_CRAP))
-		dynamic_debug_remove(info.debug);
+	dynamic_debug_remove(info.debug);
  unlock:
 	mutex_unlock(&module_mutex);
 	synchronize_sched();
-- 
1.7.7


-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.

Re: [PATCH] module: Enable dynamic debugging regardless of taint

[Rusty Russell]

On Tue, 01 Nov 2011 03:59:33 +0000, Ben Hutchings <ben@decadent.org.uk> wrote:
> Dynamic debugging is currently disabled for tainted modules, except
> for TAINT_CRAP.  This prevents use of dynamic debugging for
> out-of-tree modules now that they are also tainted.
> 
> This condition was apparently intended to avoid a crash if a force-
> loaded module has an incompatible definition of dynamic debug
> structures.  However, a administrator that forces us to load a module
> is claiming that it *is* compatible even though it fails our version
> checks.  If they are mistaken, there are any number of ways the module
> could crash the system.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Thanks, applied, unless Mathieu objects...

Cheers,
Rusty.

Re: [PATCH] module: Enable dynamic debugging regardless of taint

[Mathieu Desnoyers]

* Rusty Russell (rusty@rustcorp.com.au) wrote:
> On Tue, 01 Nov 2011 03:59:33 +0000, Ben Hutchings <ben@decadent.org.uk> wrote:
> > Dynamic debugging is currently disabled for tainted modules, except
> > for TAINT_CRAP.  This prevents use of dynamic debugging for
> > out-of-tree modules now that they are also tainted.
> > 
> > This condition was apparently intended to avoid a crash if a force-
> > loaded module has an incompatible definition of dynamic debug
> > structures.  However, a administrator that forces us to load a module
> > is claiming that it *is* compatible even though it fails our version
> > checks.  If they are mistaken, there are any number of ways the module
> > could crash the system.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> Thanks, applied, unless Mathieu objects...

I'm OK with that. We should probably note in the changelog the
side-effect of now supporting dynamic debugging of proprietary drivers.
If we start doing this for dynamic debugging, I'd be tempted to do it
for tracepoints and static jump labels too, since at least tracepoints
would not be the only "offender" (from an end-user point of view)
causing crashes on incompatible module load.

Thanks!

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

> 
> Cheers,
> Rusty.

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com