module: Re-enable dynamic debugging for GPL-compatible OOT modules

PATCH In review Ben Hutchings 3 participants 7 messages Oct 28, 2011 – Nov 1, 2011

Share & tools

Ben Hutchings

author

2011-10-28 03:38 UTC #0 lore raw

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

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 <[email protected]>

---
 kernel/module.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

kernel/module.c +2 -2

index dab585e..448fd77 100644
@@ -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);

Signature · 1 line

1.7.7

Rusty Russell

2011-10-31 06:43 UTC #1 lore raw

Quoted 3 lines · 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.

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.

Quoted 31 lines

Signed-off-by Ben Hutchings <[email protected]>

---
 kernel/module.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

kernel/module.c +2 -2

index dab585e..448fd77 100644
@@ -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);

Signature · 1 line

1.7.7

Ben Hutchings

author

2011-10-31 13:44 UTC #2 lore raw

Quoted 10 lines · Rusty Russell

Quoted 3 lines · 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.
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.

Quoted 32 lines

Quoted 31 lines

Signed-off-by Ben Hutchings <[email protected]>

---
 kernel/module.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

kernel/module.c +2 -2

index dab585e..448fd77 100644
@@ -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);

Signature · 1 line

1.7.7

Signature · 2 lines

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

Rusty Russell

2011-11-01 02:17 UTC #3 lore raw

On Mon, 31 Oct 2011 13:44:17 +0000, Ben Hutchings <[email protected]> wrote:
Non-text part: multipart/signed

Quoted 6 lines · Rusty Russell

Quoted 5 lines · Ben Hutchings
Quoted 3 lines
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.

Quoted 8 lines

Quoted 5 lines
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:

Quoted 8 lines

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.

Ben Hutchings

author

2011-11-01 03:59 UTC #4 lore raw

PATCH 1/1 module: Enable dynamic debugging regardless of taint

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 <[email protected]>

---
 kernel/module.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

kernel/module.c +2 -4

index dab585e..ef8cb70 100644
@@ -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();

Signature · 6 lines

1.7.7


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

Rusty Russell

Applied

2011-11-01 04:44 UTC #5 lore raw

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

Quoted 4 lines · 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.

Quoted 8 lines

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 <[email protected]>

Thanks, applied, unless Mathieu objects...

Cheers,
Rusty.

Mathieu Desnoyers

Acked-by

2011-11-01 12:49 UTC #6 lore raw

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

Quoted 12 lines · * Rusty Russell ([email protected]) wrote:

Quoted 11 lines · 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.

Quoted 3 lines

Quoted 1 line
Signed-off-by Ben Hutchings <[email protected]>
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 <[email protected]>

Quoted 3 lines

Cheers,
Rusty.

Signature · 4 lines

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