* [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE @ 2014-02-10 23:23 Mathieu Desnoyers 2014-02-11 7:27 ` Ingo Molnar 0 siblings, 1 reply; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-10 23:23 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Mathieu Desnoyers, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman Users have reported being unable to trace non-signed modules loaded within a kernel supporting module signature. This is caused by tracepoint.c:tracepoint_module_coming() refusing to take into account tracepoints sitting within force-loaded modules (TAINT_FORCED_MODULE). The reason for this check, in the first place, is that a force-loaded module may have a struct module incompatible with the layout expected by the kernel, and can thus cause a kernel crash upon forced load of that module on a kernel with CONFIG_TRACEPOINTS=y. Tracepoints, however, specifically accept TAINT_OOT_MODULE and TAINT_CRAP, since those modules do not lead to the "very likely system crash" issue cited above for force-loaded modules. With kernels having CONFIG_MODULE_SIG=y (signed modules), a non-signed module is tainted re-using the TAINT_FORCED_MODULE taint flag. Unfortunately, this means that Tracepoints treat that module as a force-loaded module, and thus silently refuse to consider any tracepoint within this module. Since an unsigned module does not fit within the "very likely system crash" category of tainting, add a new TAINT_UNSIGNED_MODULE taint flag to specifically address this taint behavior, and accept those modules within Tracepoints. This flag is assigned to the letter 'N', since all the more obvious ones (e.g. 'S') were already taken. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> CC: Steven Rostedt <rostedt@goodmis.org> CC: Ingo Molnar <mingo@redhat.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: Rusty Russell <rusty@rustcorp.com.au> CC: David Howells <dhowells@redhat.com> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- include/linux/kernel.h | 1 + include/trace/events/module.h | 3 ++- kernel/module.c | 4 +++- kernel/panic.c | 1 + kernel/tracepoint.c | 5 +++-- 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 196d1ea..4710900 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -469,6 +469,7 @@ extern enum system_states { #define TAINT_CRAP 10 #define TAINT_FIRMWARE_WORKAROUND 11 #define TAINT_OOT_MODULE 12 +#define TAINT_UNSIGNED_MODULE 13 extern const char hex_asc[]; #define hex_asc_lo(x) hex_asc[((x) & 0x0f)] diff --git a/include/trace/events/module.h b/include/trace/events/module.h index 1619327..1788a02 100644 --- a/include/trace/events/module.h +++ b/include/trace/events/module.h @@ -23,7 +23,8 @@ struct module; #define show_module_flags(flags) __print_flags(flags, "", \ { (1UL << TAINT_PROPRIETARY_MODULE), "P" }, \ { (1UL << TAINT_FORCED_MODULE), "F" }, \ - { (1UL << TAINT_CRAP), "C" }) + { (1UL << TAINT_CRAP), "C" }, \ + { (1UL << TAINT_UNSIGNED_MODULE), "N" }) TRACE_EVENT(module_load, diff --git a/kernel/module.c b/kernel/module.c index d24fcf2..73ca01a 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1013,6 +1013,8 @@ static size_t module_flags_taint(struct module *mod, char *buf) buf[l++] = 'F'; if (mod->taints & (1 << TAINT_CRAP)) buf[l++] = 'C'; + if (mod->taints & (1 << TAINT_UNSIGNED_MODULE)) + buf[l++] = 'N'; /* * TAINT_FORCED_RMMOD: could be added. * TAINT_UNSAFE_SMP, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't @@ -3214,7 +3216,7 @@ static int load_module(struct load_info *info, const char __user *uargs, pr_notice_once("%s: module verification failed: signature " "and/or required key missing - tainting " "kernel\n", mod->name); - add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_STILL_OK); + add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK); } #endif diff --git a/kernel/panic.c b/kernel/panic.c index 6d63003..98588e0 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -210,6 +210,7 @@ static const struct tnt tnts[] = { { TAINT_CRAP, 'C', ' ' }, { TAINT_FIRMWARE_WORKAROUND, 'I', ' ' }, { TAINT_OOT_MODULE, 'O', ' ' }, + { TAINT_UNSIGNED_MODULE, 'N', ' ' }, }; /** diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index 29f2654..e7903c1 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -639,9 +639,10 @@ static int tracepoint_module_coming(struct module *mod) /* * We skip modules that taint the kernel, especially those with different * module headers (for forced load), to make sure we don't cause a crash. - * Staging and out-of-tree GPL modules are fine. + * Staging, out-of-tree, and non-signed GPL modules are fine. */ - if (mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP))) + if (mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP) | + (1 << TAINT_UNSIGNED_MODULE))) return 0; mutex_lock(&tracepoints_mutex); tp_mod = kmalloc(sizeof(struct tp_module), GFP_KERNEL); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-10 23:23 [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE Mathieu Desnoyers @ 2014-02-11 7:27 ` Ingo Molnar 2014-02-12 4:45 ` Steven Rostedt 0 siblings, 1 reply; 36+ messages in thread From: Ingo Molnar @ 2014-02-11 7:27 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman * Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > Users have reported being unable to trace non-signed modules loaded > within a kernel supporting module signature. External modules should strive to get out of the 'crap' and 'felony law breaker' categories and we should not make it easier for them to linger in a broken state. Nacked-by: Ingo Molnar <mingo@kernel.org> Thanks, Ingo ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-11 7:27 ` Ingo Molnar @ 2014-02-12 4:45 ` Steven Rostedt 2014-02-12 5:51 ` Mathieu Desnoyers ` (2 more replies) 0 siblings, 3 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-12 4:45 UTC (permalink / raw) To: Ingo Molnar Cc: Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Tue, 11 Feb 2014 08:27:38 +0100 Ingo Molnar <mingo@kernel.org> wrote: > > * Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > Users have reported being unable to trace non-signed modules loaded > > within a kernel supporting module signature. > > External modules should strive to get out of the 'crap' and > 'felony law breaker' categories and we should not make it > easier for them to linger in a broken state. > > Nacked-by: Ingo Molnar <mingo@kernel.org> I'm not sure how great this idea is, but it isn't the same as the "crap" and "fenony law breaker" categories. Having a non-signed module doesn't mean that it isn't fully GPL compliant, it just means that it hasn't been signed. There's several things that can taint the kernel when loading a module. Being non GPL compliant is just one of them, and that will never be allowed to accept tracepoints. Forcing a module that was built for a different kernel version gives us another taint, which we don't add tracepoints for, not because it is not compliant, but because that could corrupt the kernel as we can not guarantee the binary structure layout of those modules would be the same as what the kernel was built with. We don't want people complaining about tracepoint failures due to forcing an older module into a newer kernel with different tracepoint structures. But if the kernel expects to have signed modules, and you force a module to be loaded that is not signed, then you still get that "forced" module taint, which is the same one as loading a module from an older kernel into a newer kernel. It's a different problem, and I can see having a different taint flag be more informative to kernel developers in general. I would welcome that change with or without letting tracepoints be set for that module. But I have to ask Mathieu, what exactly is the use case here? If you have a kernel that expects to only load signed modules, why would you want to force non signed ones? That basically breaks the whole purpose of signing modules. Once you allow a non signed module to be loaded then the kernel can be considered compromised. That is, you just gave kernel access to an untrusted source. -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-12 4:45 ` Steven Rostedt @ 2014-02-12 5:51 ` Mathieu Desnoyers 2014-02-13 3:24 ` Rusty Russell 2014-02-13 15:10 ` Mathieu Desnoyers 2 siblings, 0 replies; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-12 5:51 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Ingo Molnar" <mingo@kernel.org> > Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" > <mingo@redhat.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" > <dhowells@redhat.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Tuesday, February 11, 2014 11:45:34 PM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > On Tue, 11 Feb 2014 08:27:38 +0100 > Ingo Molnar <mingo@kernel.org> wrote: > > > > > * Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > > Users have reported being unable to trace non-signed modules loaded > > > within a kernel supporting module signature. > > > > External modules should strive to get out of the 'crap' and > > 'felony law breaker' categories and we should not make it > > easier for them to linger in a broken state. > > > > Nacked-by: Ingo Molnar <mingo@kernel.org> > > I'm not sure how great this idea is, but it isn't the same as the > "crap" and "fenony law breaker" categories. Having a non-signed module > doesn't mean that it isn't fully GPL compliant, it just means that it > hasn't been signed. There's several things that can taint the kernel > when loading a module. Being non GPL compliant is just one of them, and > that will never be allowed to accept tracepoints. > > Forcing a module that was built for a different kernel version gives us > another taint, which we don't add tracepoints for, not because it is > not compliant, but because that could corrupt the kernel as we can > not guarantee the binary structure layout of those modules would be the > same as what the kernel was built with. We don't want people > complaining about tracepoint failures due to forcing an older module > into a newer kernel with different tracepoint structures. > > But if the kernel expects to have signed modules, and you force a > module to be loaded that is not signed, then you still get that > "forced" module taint, which is the same one as loading a module from > an older kernel into a newer kernel. It's a different problem, and I > can see having a different taint flag be more informative to kernel > developers in general. I would welcome that change with or without > letting tracepoints be set for that module. > > But I have to ask Mathieu, what exactly is the use case here? If you > have a kernel that expects to only load signed modules, why would you > want to force non signed ones? That basically breaks the whole purpose > of signing modules. Once you allow a non signed module to be loaded > then the kernel can be considered compromised. That is, you just gave > kernel access to an untrusted source. The use-case is with a kernel that has this config: CONFIG_MODULE_SIG=y # CONFIG_MODULE_SIG_FORCE is not set which is the case for at least Ubuntu kernels (that I know of). It allows users to specify the kernel boot argument "module.sig_enforce" if they care about refusing unsigned modules. The use-case targeted here is loading GPL compliant out-of-tree modules with those kernels, obviously not using the kernel boot argument "module.sig_enforce". Tracepoints contained within those modules are silently skipped due to the TAINT_FORCED_MODULE flag. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-12 4:45 ` Steven Rostedt 2014-02-12 5:51 ` Mathieu Desnoyers @ 2014-02-13 3:24 ` Rusty Russell 2014-02-13 21:11 ` Steven Rostedt 2014-02-13 15:10 ` Mathieu Desnoyers 2 siblings, 1 reply; 36+ messages in thread From: Rusty Russell @ 2014-02-13 3:24 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar Cc: Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman Steven Rostedt <rostedt@goodmis.org> writes: > On Tue, 11 Feb 2014 08:27:38 +0100 > Ingo Molnar <mingo@kernel.org> wrote: > >> >> * Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: >> >> > Users have reported being unable to trace non-signed modules loaded >> > within a kernel supporting module signature. >> >> External modules should strive to get out of the 'crap' and >> 'felony law breaker' categories and we should not make it >> easier for them to linger in a broken state. >> >> Nacked-by: Ingo Molnar <mingo@kernel.org> Well, distributions which sign their modules are sending a pretty strong "go away" signal already. I'm ambivalent towards out-of-tree modules, so not tempted unless I see a bug report indicating a concrete problem. Then we can discuss... Cheers, Rusty. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 3:24 ` Rusty Russell @ 2014-02-13 21:11 ` Steven Rostedt 2014-02-13 21:24 ` Steven Rostedt 2014-02-14 0:51 ` Rusty Russell 0 siblings, 2 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-13 21:11 UTC (permalink / raw) To: Rusty Russell Cc: Ingo Molnar, Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman On Thu, 13 Feb 2014 13:54:42 +1030 Rusty Russell <rusty@rustcorp.com.au> wrote: > I'm ambivalent towards out-of-tree modules, so not tempted unless I see > a bug report indicating a concrete problem. Then we can discuss... As I replied in another email, this is a concrete problem, and affects in-tree kernel modules. If you have the following in your .config: CONFIG_MODULE_SIG=y # CONFIG_MODULE_SIG_FORCE is not set # CONFIG_MODULE_SIG_ALL is not set Modules will not be signed at build, and they can be loaded with a simple modprobe or insmod with no --force flag set. You may get an error message like: sunrpc: module verification failed: signature and/or required key missing - tainting kernel But nothing else that indicates a problem. In the module code, the above was printed by: #ifdef CONFIG_MODULE_SIG mod->sig_ok = info->sig_ok; if (!mod->sig_ok) { pr_notice_once("%s: module verification failed: signature " "and/or required key missing - tainting " "kernel\n", mod->name); add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_STILL_OK); } #endif Now in the tracepoint code, we have: in tracepoint_module_coming(): if (mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP))) return 0; If the module is tainted as other than out-of-tree or crap (staging), the module is ignored with respect to tracepoints. No error, no nothing. This means that all modules loaded with the config will not have their tracepoints enabled. I highly doubt this is the expected result. I think Mathieu's patch is a fix to this problem (and my patch fixes the problem where tracepoints do not give any feedback that they failed to be enabled). Are you fine with his fix, if so, please ack it, and I'll apply it. Although, is "N" the best letter to use for this taint? Not sure, but everything else I can think of looks to be already taken. Maybe "X"? You know. When you sign your name and don't know how to spell it, you just simply use an "X". :-) Thanks! -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 21:11 ` Steven Rostedt @ 2014-02-13 21:24 ` Steven Rostedt 2014-02-14 3:32 ` Mathieu Desnoyers 2014-02-14 0:51 ` Rusty Russell 1 sibling, 1 reply; 36+ messages in thread From: Steven Rostedt @ 2014-02-13 21:24 UTC (permalink / raw) To: Steven Rostedt Cc: Rusty Russell, Ingo Molnar, Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman On Thu, 13 Feb 2014 16:11:56 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > Although, is "N" the best letter to use for this taint? Not sure, but > everything else I can think of looks to be already taken. Maybe "X"? > You know. When you sign your name and don't know how to spell it, you > just simply use an "X". :-) I actually think "X" is appropriate. You want signed modules, but the module being loaded doesn't know how to sign its name, so we simply use an "X" for it (in the taint flag). -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 21:24 ` Steven Rostedt @ 2014-02-14 3:32 ` Mathieu Desnoyers 0 siblings, 0 replies; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-14 3:32 UTC (permalink / raw) To: Steven Rostedt Cc: Rusty Russell, Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Steven Rostedt" <rostedt@goodmis.org> > Cc: "Rusty Russell" <rusty@rustcorp.com.au>, "Ingo Molnar" <mingo@kernel.org>, "Mathieu Desnoyers" > <mathieu.desnoyers@efficios.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas Gleixner" > <tglx@linutronix.de>, "David Howells" <dhowells@redhat.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Thursday, February 13, 2014 4:24:31 PM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > On Thu, 13 Feb 2014 16:11:56 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > Although, is "N" the best letter to use for this taint? Not sure, but > > everything else I can think of looks to be already taken. Maybe "X"? > > You know. When you sign your name and don't know how to spell it, you > > just simply use an "X". :-) > > I actually think "X" is appropriate. You want signed modules, but the > module being loaded doesn't know how to sign its name, so we simply use > an "X" for it (in the taint flag). I like the "X" idea :) Will prepare an updated patch with it. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 21:11 ` Steven Rostedt 2014-02-13 21:24 ` Steven Rostedt @ 2014-02-14 0:51 ` Rusty Russell 2014-02-16 23:58 ` Mathieu Desnoyers 2014-02-20 15:30 ` Steven Rostedt 1 sibling, 2 replies; 36+ messages in thread From: Rusty Russell @ 2014-02-14 0:51 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman Steven Rostedt <rostedt@goodmis.org> writes: > On Thu, 13 Feb 2014 13:54:42 +1030 > Rusty Russell <rusty@rustcorp.com.au> wrote: > > >> I'm ambivalent towards out-of-tree modules, so not tempted unless I see >> a bug report indicating a concrete problem. Then we can discuss... > > As I replied in another email, this is a concrete problem, and affects > in-tree kernel modules. > > If you have the following in your .config: > > CONFIG_MODULE_SIG=y > # CONFIG_MODULE_SIG_FORCE is not set > # CONFIG_MODULE_SIG_ALL is not set This means you've set the "I will arrange my own module signing" config option: Sign all modules during make modules_install. Without this option, modules must be signed manually, using the scripts/sign-file tool. comment "Do not forget to sign required modules with scripts/sign-file" depends on MODULE_SIG_FORCE && !MODULE_SIG_ALL Then you didn't do that. You broke it, you get to keep both pieces. Again: is there an actual valid use case? Rusty. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-14 0:51 ` Rusty Russell @ 2014-02-16 23:58 ` Mathieu Desnoyers 2014-02-20 15:30 ` Steven Rostedt 1 sibling, 0 replies; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-16 23:58 UTC (permalink / raw) To: Rusty Russell Cc: Steven Rostedt, Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Rusty Russell" <rusty@rustcorp.com.au> > To: "Steven Rostedt" <rostedt@goodmis.org> > Cc: "Ingo Molnar" <mingo@kernel.org>, "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, > linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas Gleixner" <tglx@linutronix.de>, "David > Howells" <dhowells@redhat.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Thursday, February 13, 2014 7:51:19 PM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > Steven Rostedt <rostedt@goodmis.org> writes: > > On Thu, 13 Feb 2014 13:54:42 +1030 > > Rusty Russell <rusty@rustcorp.com.au> wrote: > > > > > >> I'm ambivalent towards out-of-tree modules, so not tempted unless I see > >> a bug report indicating a concrete problem. Then we can discuss... > > > > As I replied in another email, this is a concrete problem, and affects > > in-tree kernel modules. > > > > If you have the following in your .config: > > > > CONFIG_MODULE_SIG=y > > # CONFIG_MODULE_SIG_FORCE is not set > > # CONFIG_MODULE_SIG_ALL is not set > > This means you've set the "I will arrange my own module signing" config > option: > > Sign all modules during make modules_install. Without this option, > modules must be signed manually, using the scripts/sign-file tool. > > comment "Do not forget to sign required modules with scripts/sign-file" > depends on MODULE_SIG_FORCE && !MODULE_SIG_ALL > > Then you didn't do that. You broke it, you get to keep both pieces. > > Again: is there an actual valid use case? One use-case where this is biting us for in-tree modules is when a user or developer recompile modules against a distribution kernel which has CONFIG_MODULE_SIG set (and possibly CONFIG_MODULE_SIG_ALL), but do not recompile the kernel per se. That user/developer might want to try out a local modification to one of his modules (which is something within the user's rights given by the GPL), or want to add tracepoints to a module to figure out what is going wrong. It is then not possible to sign the recompiled modules, since it makes no sense to expect distribution vendors to ever distribute their private signing keys; that would defeat the whole point of signing. In those cases, when loaded in a kernel that is not enforcing module signature, the recompiled modules will taint the kernel and modules with "TAINT_FORCED_MODULE" (which is a lie: the modules can be loaded without --force), and the tracepoints sitting in that module are silently ignored (which is a bug). Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-14 0:51 ` Rusty Russell 2014-02-16 23:58 ` Mathieu Desnoyers @ 2014-02-20 15:30 ` Steven Rostedt 2014-02-20 23:09 ` Rusty Russell 1 sibling, 1 reply; 36+ messages in thread From: Steven Rostedt @ 2014-02-20 15:30 UTC (permalink / raw) To: Rusty Russell Cc: Ingo Molnar, Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman I need to clean out my email box. This email was hidden in between a pile of other crap email. On Fri, 14 Feb 2014 11:21:19 +1030 Rusty Russell <rusty@rustcorp.com.au> wrote: > Steven Rostedt <rostedt@goodmis.org> writes: > > On Thu, 13 Feb 2014 13:54:42 +1030 > > Rusty Russell <rusty@rustcorp.com.au> wrote: > > > > > >> I'm ambivalent towards out-of-tree modules, so not tempted unless I see > >> a bug report indicating a concrete problem. Then we can discuss... > > > > As I replied in another email, this is a concrete problem, and affects > > in-tree kernel modules. > > > > If you have the following in your .config: > > > > CONFIG_MODULE_SIG=y > > # CONFIG_MODULE_SIG_FORCE is not set > > # CONFIG_MODULE_SIG_ALL is not set > > This means you've set the "I will arrange my own module signing" config > option: > > Sign all modules during make modules_install. Without this option, > modules must be signed manually, using the scripts/sign-file tool. > > comment "Do not forget to sign required modules with scripts/sign-file" > depends on MODULE_SIG_FORCE && !MODULE_SIG_ALL > > Then you didn't do that. You broke it, you get to keep both pieces. In this case we should fail the module load all together, and require insmod to add the --force flag to load it. Why the hell are we setting a FORCED_MODULE flag when no module was forced???? -- Steve > > Again: is there an actual valid use case? > Rusty. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-20 15:30 ` Steven Rostedt @ 2014-02-20 23:09 ` Rusty Russell 2014-02-21 4:09 ` Steven Rostedt 0 siblings, 1 reply; 36+ messages in thread From: Rusty Russell @ 2014-02-20 23:09 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman Steven Rostedt <rostedt@goodmis.org> writes: > I need to clean out my email box. This email was hidden in between a > pile of other crap email. > > On Fri, 14 Feb 2014 11:21:19 +1030 > Rusty Russell <rusty@rustcorp.com.au> wrote: > >> Steven Rostedt <rostedt@goodmis.org> writes: >> > On Thu, 13 Feb 2014 13:54:42 +1030 >> > Rusty Russell <rusty@rustcorp.com.au> wrote: >> > >> > >> >> I'm ambivalent towards out-of-tree modules, so not tempted unless I see >> >> a bug report indicating a concrete problem. Then we can discuss... >> > >> > As I replied in another email, this is a concrete problem, and affects >> > in-tree kernel modules. >> > >> > If you have the following in your .config: >> > >> > CONFIG_MODULE_SIG=y >> > # CONFIG_MODULE_SIG_FORCE is not set >> > # CONFIG_MODULE_SIG_ALL is not set >> >> This means you've set the "I will arrange my own module signing" config >> option: >> >> Sign all modules during make modules_install. Without this option, >> modules must be signed manually, using the scripts/sign-file tool. >> >> comment "Do not forget to sign required modules with scripts/sign-file" >> depends on MODULE_SIG_FORCE && !MODULE_SIG_ALL >> >> Then you didn't do that. You broke it, you get to keep both pieces. > > In this case we should fail the module load all together, and require > insmod to add the --force flag to load it. Why the hell are we setting > a FORCED_MODULE flag when no module was forced???? If this mistake of creating unsigned modules is common, then it would be friendly to do something about it, yes. Perhaps we should append UNSIGNED to vermagic, and then strip that out when we sign the module? That would have this effect. Cheers, Rusty. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-20 23:09 ` Rusty Russell @ 2014-02-21 4:09 ` Steven Rostedt 2014-02-21 8:10 ` Johannes Berg 0 siblings, 1 reply; 36+ messages in thread From: Steven Rostedt @ 2014-02-21 4:09 UTC (permalink / raw) To: Rusty Russell Cc: Ingo Molnar, Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman, Johannes Berg On Fri, 21 Feb 2014 09:39:18 +1030 Rusty Russell <rusty@rustcorp.com.au> wrote: > >> comment "Do not forget to sign required modules with scripts/sign-file" > >> depends on MODULE_SIG_FORCE && !MODULE_SIG_ALL > >> > >> Then you didn't do that. You broke it, you get to keep both pieces. > > > > In this case we should fail the module load all together, and require > > insmod to add the --force flag to load it. Why the hell are we setting > > a FORCED_MODULE flag when no module was forced???? > > If this mistake of creating unsigned modules is common, then it would be > friendly to do something about it, yes. I just got another report about it happening again today. > > Perhaps we should append UNSIGNED to vermagic, and then strip that out > when we sign the module? That would have this effect. If it makes the user have to use --force, then they will be more aware something went wrong when things are not working. I still find it strange that things like tracepoints are not working on a module just because it wasn't signed. I can see someone adding something and not doing the signing, and wonder why tracepoints are broken. Right now, I'm the one that gets the bug reports, not you :-( Them: "Tracepoints are broken". Me: "What do you mean tracepoints are broken?" Them: "I enabled them, but don't see them being recorded" Me: "Is this for tracepoints in a module?" Them: "Yes" Me: "Oh, this again. Do you have MODULE_SIG_FORCE & !MODULE_SIG_ALL set, and you didn't sign your module?" Them: "Yes" Me: "Well that's your problem." Them: "What does that have to do with tracepoints?" Me: "Well, loading a non signed module into a kernel that requires sigs, sets the FORCED_MODULE flag, and that breaks tracepoints" Them: "Why? The module is from the same kernel and built with the same tool chain" Me: "it just does" Them: "I'm just working on this module, and I want to trace it, but don't want to keep signing it" Me: "Oh well, that's just the way it goes" Them: "Why?" When honestly, just adding another taint flag and let tracepoints still load would keep people from pestering me about it. The only reason we don't trace FORCED_MODULE modules, is because it may have an older data structure, or different tracepoint logic. We avoid adding tracepoints on those because we don't want bug reports from people using tracepoints in modules that were compiled for a different kernel, because that could cause havoc. Ironically, having tracepoints not added for modules with that taint is causing more bug reports due to this signing issue. Maybe I'll just let tracepoints work with modules that have that taint and deal with the bug reports that are caused by people loading older modules into newer kernels. -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-21 4:09 ` Steven Rostedt @ 2014-02-21 8:10 ` Johannes Berg 2014-02-26 2:51 ` Rusty Russell 0 siblings, 1 reply; 36+ messages in thread From: Johannes Berg @ 2014-02-21 8:10 UTC (permalink / raw) To: Steven Rostedt Cc: Rusty Russell, Ingo Molnar, Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman On Thu, 2014-02-20 at 23:09 -0500, Steven Rostedt wrote: > On Fri, 21 Feb 2014 09:39:18 +1030 > Rusty Russell <rusty@rustcorp.com.au> wrote: > > > >> comment "Do not forget to sign required modules with scripts/sign-file" > > >> depends on MODULE_SIG_FORCE && !MODULE_SIG_ALL > > >> > > >> Then you didn't do that. You broke it, you get to keep both pieces. > > > > > > In this case we should fail the module load all together, and require > > > insmod to add the --force flag to load it. Why the hell are we setting > > > a FORCED_MODULE flag when no module was forced???? > > > > If this mistake of creating unsigned modules is common, then it would be > > friendly to do something about it, yes. > > I just got another report about it happening again today. Not sure if you meant me, but it was only indirectly reported to me as well (I get to be considered the resident tracing "expert" in our team), and my colleague spent two or three days trying to figure this out before getting me involved, and when I couldn't figure it out either I asked Steve ... So yeah, there's definitely potential for confusion here, *particularly* since there's no message, and the module shows as "forced taint" which (as has been described elsewhere in the thread) is particularly confusing since no forced loading was done. > > Perhaps we should append UNSIGNED to vermagic, and then strip that out > > when we sign the module? That would have this effect. > > If it makes the user have to use --force, then they will be more aware > something went wrong when things are not working. That's one way of looking at it, but why should a kernel that doesn't require module signing require a --force flag for loading an unsigned module? To me, that's chickening out - either you support loading unsigned modules or you don't. Having half-baked support for it makes no sense, IMHO. After all, given the configuration (no signature requirement) there's nothing wrong with the module. Additionally, it seems to me that you could still sign a module and then --force it (ending up with a forced taint) so you can't actually tell which one you did. Thus, if you sit in front of such a machine, you have no way of figuring out what actually happened. > I still find it strange that things like tracepoints are not working on > a module just because it wasn't signed. I can see someone adding > something and not doing the signing, and wonder why tracepoints are > broken. Right now, I'm the one that gets the bug reports, not you :-( > When honestly, just adding another taint flag and let tracepoints > still load would keep people from pestering me about it. The mere existence of a configuration to allow unsigned modules would indicate that there are valid use cases for that (and rebuilding a module or such for development would seem to be one of them), so why would tracing be impacted, particularly for development. > The only reason we don't trace FORCED_MODULE modules, is because it may > have an older data structure, or different tracepoint logic. We avoid > adding tracepoints on those because we don't want bug reports from > people using tracepoints in modules that were compiled for a different > kernel, because that could cause havoc. Ironically, having tracepoints > not added for modules with that taint is causing more bug reports due > to this signing issue. Maybe I'll just let tracepoints work with modules > that have that taint and deal with the bug reports that are caused by > people loading older modules into newer kernels. I'd much prefer to split the taints, given that then it would also give us a way to distinguish between * signed module loaded with --force * unsigned module loaded Going on a tangent here - our use case is using backported upstream kernel modules (https://backports.wiki.kernel.org/) for delivering a driver to people who decided that they absolutely need to run with some random kernel (e.g. 3.10) but we don't yet support all the driver features they want/need in the kernel they picked. We push our code upstream as soon as we can and typically only diverge from upstream by a few patches, so saying things like "crap" or "felony law breaker" about out-of-tree modules in general makes me furious. johannes ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-21 8:10 ` Johannes Berg @ 2014-02-26 2:51 ` Rusty Russell 2014-02-26 12:55 ` Mathieu Desnoyers 0 siblings, 1 reply; 36+ messages in thread From: Rusty Russell @ 2014-02-26 2:51 UTC (permalink / raw) To: Johannes Berg, Steven Rostedt Cc: Ingo Molnar, Mathieu Desnoyers, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman Johannes Berg <johannes@sipsolutions.net> writes: > Going on a tangent here - our use case is using backported upstream > kernel modules (https://backports.wiki.kernel.org/) for delivering a > driver to people who decided that they absolutely need to run with some > random kernel (e.g. 3.10) but we don't yet support all the driver > features they want/need in the kernel they picked. Ah, a user! See, that's not the "I forgot to sign my modules" case the others were complaining about. > We push our code upstream as soon as we can and typically only diverge > from upstream by a few patches, so saying things like "crap" or "felony > law breaker" about out-of-tree modules in general makes me furious. Appreciated and understood. I have applied Mathieu's patch to my pending tree, with Ingo's Nack recorded. Thanks, Rusty. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-26 2:51 ` Rusty Russell @ 2014-02-26 12:55 ` Mathieu Desnoyers 0 siblings, 0 replies; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-26 12:55 UTC (permalink / raw) To: Rusty Russell Cc: Johannes Berg, Steven Rostedt, Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Rusty Russell" <rusty@rustcorp.com.au> > To: "Johannes Berg" <johannes@sipsolutions.net>, "Steven Rostedt" <rostedt@goodmis.org> > Cc: "Ingo Molnar" <mingo@kernel.org>, "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, > linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas Gleixner" <tglx@linutronix.de>, "David > Howells" <dhowells@redhat.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Tuesday, February 25, 2014 9:51:50 PM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > Johannes Berg <johannes@sipsolutions.net> writes: > > Going on a tangent here - our use case is using backported upstream > > kernel modules (https://backports.wiki.kernel.org/) for delivering a > > driver to people who decided that they absolutely need to run with some > > random kernel (e.g. 3.10) but we don't yet support all the driver > > features they want/need in the kernel they picked. > > Ah, a user! See, that's not the "I forgot to sign my modules" case the > others were complaining about. > > > We push our code upstream as soon as we can and typically only diverge > > from upstream by a few patches, so saying things like "crap" or "felony > > law breaker" about out-of-tree modules in general makes me furious. > > Appreciated and understood. > > I have applied Mathieu's patch to my pending tree, with Ingo's Nack > recorded. Hi Rusty, That RFC patch was superseded by a newer version, posted in a separate thread. There were documentation and other printout sites to update. I posted the non-RFC version of the patch here: https://lkml.org/lkml/2014/2/14/4 (you should have it in your inbox) Please let me know if I need to repost it. Thanks! Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-12 4:45 ` Steven Rostedt 2014-02-12 5:51 ` Mathieu Desnoyers 2014-02-13 3:24 ` Rusty Russell @ 2014-02-13 15:10 ` Mathieu Desnoyers 2014-02-13 15:28 ` Steven Rostedt 2 siblings, 1 reply; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-13 15:10 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Ingo Molnar" <mingo@kernel.org> > Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" > <mingo@redhat.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" > <dhowells@redhat.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Tuesday, February 11, 2014 11:45:34 PM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > [...] > But if the kernel expects to have signed modules, and you force a > module to be loaded that is not signed, then you still get that > "forced" module taint, which is the same one as loading a module from > an older kernel into a newer kernel. It's a different problem, and I > can see having a different taint flag be more informative to kernel > developers in general. I would welcome that change with or without > letting tracepoints be set for that module. There is one important inaccuracy in your explanation above: a kernel supporting signed modules, but not enforcing "sig_force", can load unsigned modules with a simple modprobe or insmod, without any "--force" argument. Therefore, tainting the module as "TAINT_FORCED_MODULE" is misleading. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 15:10 ` Mathieu Desnoyers @ 2014-02-13 15:28 ` Steven Rostedt 2014-02-13 15:36 ` Frank Ch. Eigler 2014-02-13 15:41 ` Mathieu Desnoyers 0 siblings, 2 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-13 15:28 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Thu, 13 Feb 2014 15:10:14 +0000 (UTC) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > ----- Original Message ----- > > From: "Steven Rostedt" <rostedt@goodmis.org> > > To: "Ingo Molnar" <mingo@kernel.org> > > Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" > > <mingo@redhat.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" > > <dhowells@redhat.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > > Sent: Tuesday, February 11, 2014 11:45:34 PM > > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > > > > [...] > > But if the kernel expects to have signed modules, and you force a > > module to be loaded that is not signed, then you still get that > > "forced" module taint, which is the same one as loading a module from > > an older kernel into a newer kernel. It's a different problem, and I > > can see having a different taint flag be more informative to kernel > > developers in general. I would welcome that change with or without > > letting tracepoints be set for that module. > > There is one important inaccuracy in your explanation above: a > kernel supporting signed modules, but not enforcing "sig_force", > can load unsigned modules with a simple modprobe or insmod, without > any "--force" argument. Therefore, tainting the module as > "TAINT_FORCED_MODULE" is misleading. > Oh! You are saying that if the kernel only *supports* signed modules, and you load a module that is not signed, it will taint the kernel? -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 15:28 ` Steven Rostedt @ 2014-02-13 15:36 ` Frank Ch. Eigler 2014-02-13 15:44 ` Steven Rostedt 2014-02-13 15:41 ` Mathieu Desnoyers 1 sibling, 1 reply; 36+ messages in thread From: Frank Ch. Eigler @ 2014-02-13 15:36 UTC (permalink / raw) To: Steven Rostedt Cc: Mathieu Desnoyers, Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman rostedt wrote: > [...] > Oh! You are saying that if the kernel only *supports* signed modules, > and you load a module that is not signed, it will taint the kernel? Yes: this is the default for several distros. - FChE ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 15:36 ` Frank Ch. Eigler @ 2014-02-13 15:44 ` Steven Rostedt 2014-02-13 21:42 ` Arend van Spriel 0 siblings, 1 reply; 36+ messages in thread From: Steven Rostedt @ 2014-02-13 15:44 UTC (permalink / raw) To: Frank Ch. Eigler Cc: Mathieu Desnoyers, Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Thu, 13 Feb 2014 10:36:35 -0500 fche@redhat.com (Frank Ch. Eigler) wrote: > > rostedt wrote: > > > [...] > > Oh! You are saying that if the kernel only *supports* signed modules, > > and you load a module that is not signed, it will taint the kernel? > > Yes: this is the default for several distros. > Rusty, Ingo, This looks like a bug to me, as it can affect even in-tree kernel modules. If you have a kernel that supports signed modules, and you modify a module, recompile it, apply it, since it is no longer signed, then it sounds like we just tainted it. Worse yet, we just disabled any tracepoints on that module, which means it is even harder to debug that module (if that's the reason you recompiled it in the first place). -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 15:44 ` Steven Rostedt @ 2014-02-13 21:42 ` Arend van Spriel 0 siblings, 0 replies; 36+ messages in thread From: Arend van Spriel @ 2014-02-13 21:42 UTC (permalink / raw) To: Steven Rostedt, Frank Ch. Eigler Cc: Mathieu Desnoyers, Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On 02/13/2014 04:44 PM, Steven Rostedt wrote: > On Thu, 13 Feb 2014 10:36:35 -0500 > fche@redhat.com (Frank Ch. Eigler) wrote: > >> >> rostedt wrote: >> >>> [...] >>> Oh! You are saying that if the kernel only *supports* signed modules, >>> and you load a module that is not signed, it will taint the kernel? >> >> Yes: this is the default for several distros. >> > > Rusty, Ingo, > > This looks like a bug to me, as it can affect even in-tree kernel > modules. If you have a kernel that supports signed modules, and you > modify a module, recompile it, apply it, since it is no longer signed, > then it sounds like we just tainted it. Worse yet, we just disabled any > tracepoints on that module, which means it is even harder to debug that > module (if that's the reason you recompiled it in the first place). When I stumbled upon this issue a while ago on Fedora 19 I built my kernel rpm packages which generates a signature key (.priv and .x509), which I kept safe with the kernel headers. When building recompiling modules I refer to it with MODSECKEY and MODPUBKEY, ie. $ make MODSECKEY=bla MODPUBKEY=duh \ M=drivers/net/wireless/brcm80211 modules Or sign it manually using the sign-file perl script: mod_sign_cmd = perl $(srctree)/scripts/sign-file \ $(CONFIG_MODULE_SIG_HASH) $(MODSECKEY) $(MODPUBKEY) Of course I could disable signed modules while building a new kernel, but I was in it for the ride (I had better ones) ;-) Gr. AvS > -- Steve > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 15:28 ` Steven Rostedt 2014-02-13 15:36 ` Frank Ch. Eigler @ 2014-02-13 15:41 ` Mathieu Desnoyers 2014-02-13 20:45 ` Steven Rostedt 1 sibling, 1 reply; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-13 15:41 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" <dhowells@redhat.com>, > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Thursday, February 13, 2014 10:28:17 AM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > On Thu, 13 Feb 2014 15:10:14 +0000 (UTC) > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > ----- Original Message ----- > > > From: "Steven Rostedt" <rostedt@goodmis.org> > > > To: "Ingo Molnar" <mingo@kernel.org> > > > Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, > > > linux-kernel@vger.kernel.org, "Ingo Molnar" > > > <mingo@redhat.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Rusty > > > Russell" <rusty@rustcorp.com.au>, "David Howells" > > > <dhowells@redhat.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > > > Sent: Tuesday, February 11, 2014 11:45:34 PM > > > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new > > > TAINT_UNSIGNED_MODULE > > > > > > > > [...] > > > But if the kernel expects to have signed modules, and you force a > > > module to be loaded that is not signed, then you still get that > > > "forced" module taint, which is the same one as loading a module from > > > an older kernel into a newer kernel. It's a different problem, and I > > > can see having a different taint flag be more informative to kernel > > > developers in general. I would welcome that change with or without > > > letting tracepoints be set for that module. > > > > There is one important inaccuracy in your explanation above: a > > kernel supporting signed modules, but not enforcing "sig_force", > > can load unsigned modules with a simple modprobe or insmod, without > > any "--force" argument. Therefore, tainting the module as > > "TAINT_FORCED_MODULE" is misleading. > > > > Oh! You are saying that if the kernel only *supports* signed modules, > and you load a module that is not signed, it will taint the kernel? Yes, exactly, presuming that by "supporting" you mean CONFIG_MODULE_SIG=y. Loading an unsigned module then taints the kernel, and taints the module with TAINT_FORCED_MODULE even though "modprobe --force" was never used. Thanks, Mathieu > > > -- Steve > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 15:41 ` Mathieu Desnoyers @ 2014-02-13 20:45 ` Steven Rostedt 2014-02-14 3:49 ` Mathieu Desnoyers 0 siblings, 1 reply; 36+ messages in thread From: Steven Rostedt @ 2014-02-13 20:45 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Thu, 13 Feb 2014 15:41:30 +0000 (UTC) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > Yes, exactly, presuming that by "supporting" you mean CONFIG_MODULE_SIG=y. > Loading an unsigned module then taints the kernel, and taints the module > with TAINT_FORCED_MODULE even though "modprobe --force" was never used. OK, this IS a major bug and needs to be fixed. This explains a couple of reports I received about tracepoints not working, and I never figured out why. Basically, they even did this: trace_printk("before tracepoint\n"); trace_some_trace_point(); trace_printk("after tracepoint\n"); Enabled the tracepoint (it shows up as enabled and working in the tools, but not the trace), but in the trace they would get: before tracepoint after tracepoint and never get the actual tracepoint. But as they were debugging something else, it was just thought that this was their bug. But it baffled me to why that tracepoint wasn't working even though nothing in the dmesg had any errors about tracepoints. Well, this now explains it. If you compile a kernel with the following options: CONFIG_MODULE_SIG=y # CONFIG_MODULE_SIG_FORCE is not set # CONFIG_MODULE_SIG_ALL is not set You now just disabled (silently) all tracepoints in modules. WITH NO FREAKING ERROR MESSAGE!!! The tracepoints will show up in /sys/kernel/debug/tracing/events, they will show up in perf list, you can enable them in either perf or the debugfs, but they will never actually be executed. You will just get silence even though everything appeared to be working just fine. I tested this out. I was not able to get any tracepoints in modules working with the above config options. There's two bugs here. One, the lack of MODULE_SIG_ALL, will make all modules non signed during the build process. That means that all modules when loaded will be tainted as FORCED. As Mathieu stated, you do not need the --force flag here, it just needs to see that the kernel supports signatures but the module is not signed. In this case, it just calls add_taint_module(), tainting the module with FORCED_MODULE. You get a message like this: sunrpc: module verification failed: signature and/or required key missing - tainting kernel Now when this module adds its tracepoint, since it is marked with a FORCE_MODULE taint flag, the tracepoint code just ignores it and does not do any processing at all. Worse yet, the tracepoint code never gives any feedback if a tracepoint was enabled or not. That is, if a tracepoint was never initialized when the module was loaded, the tracepoint will still report success at time of enabling, that it was registered (and tracing) even though it is not. At the end of this email, I added a patch that fixes that. But I have to concur that Mathieu did find a bug. There is no reason to disable tracepoints in modules if someone simply has the above configs enabled (and disabled)! Below is the patch that warns if the tracepoint is not enabled for whatever reason. Signed-off-by: Steven Rostedt <rostedt@goodmis.org> -- Steve diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index 29f2654..b85f68f 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -62,6 +62,7 @@ struct tracepoint_entry { struct hlist_node hlist; struct tracepoint_func *funcs; int refcount; /* Number of times armed. 0 if disarmed. */ + int enabled; /* Tracepoint enabled */ char name[0]; }; @@ -237,6 +238,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name) memcpy(&e->name[0], name, name_len); e->funcs = NULL; e->refcount = 0; + e->enabled = 0; hlist_add_head(&e->hlist, head); return e; } @@ -316,6 +318,7 @@ static void tracepoint_update_probe_range(struct tracepoint * const *begin, if (mark_entry) { set_tracepoint(&mark_entry, *iter, !!mark_entry->refcount); + mark_entry->enabled = !!mark_entry->refcount; } else { disable_tracepoint(*iter); } @@ -380,6 +383,8 @@ tracepoint_add_probe(const char *name, void *probe, void *data) int tracepoint_probe_register(const char *name, void *probe, void *data) { struct tracepoint_func *old; + struct tracepoint_entry *entry; + int ret = 0; mutex_lock(&tracepoints_mutex); old = tracepoint_add_probe(name, probe, data); @@ -388,9 +393,15 @@ int tracepoint_probe_register(const char *name, void *probe, void *data) return PTR_ERR(old); } tracepoint_update_probes(); /* may update entry */ + entry = get_tracepoint(name); + /* Make sure the entry was enabled */ + if (!entry || !entry->enabled) { + tracepoint_entry_remove_probe(entry, probe, data); + ret = -ENODEV; + } mutex_unlock(&tracepoints_mutex); release_probes(old); - return 0; + return ret; } EXPORT_SYMBOL_GPL(tracepoint_probe_register); ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-13 20:45 ` Steven Rostedt @ 2014-02-14 3:49 ` Mathieu Desnoyers 2014-02-24 15:54 ` Steven Rostedt 0 siblings, 1 reply; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-14 3:49 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" <dhowells@redhat.com>, > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Thursday, February 13, 2014 3:45:07 PM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > On Thu, 13 Feb 2014 15:41:30 +0000 (UTC) > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > Yes, exactly, presuming that by "supporting" you mean CONFIG_MODULE_SIG=y. > > Loading an unsigned module then taints the kernel, and taints the module > > with TAINT_FORCED_MODULE even though "modprobe --force" was never used. > > OK, this IS a major bug and needs to be fixed. This explains a couple > of reports I received about tracepoints not working, and I never > figured out why. Basically, they even did this: > > > trace_printk("before tracepoint\n"); > trace_some_trace_point(); > trace_printk("after tracepoint\n"); > > Enabled the tracepoint (it shows up as enabled and working in the > tools, but not the trace), but in the trace they would get: > > before tracepoint > after tracepoint > > and never get the actual tracepoint. But as they were debugging > something else, it was just thought that this was their bug. But it > baffled me to why that tracepoint wasn't working even though nothing in > the dmesg had any errors about tracepoints. > > Well, this now explains it. If you compile a kernel with the following > options: > > CONFIG_MODULE_SIG=y > # CONFIG_MODULE_SIG_FORCE is not set > # CONFIG_MODULE_SIG_ALL is not set > > You now just disabled (silently) all tracepoints in modules. WITH NO > FREAKING ERROR MESSAGE!!! > > The tracepoints will show up in /sys/kernel/debug/tracing/events, they > will show up in perf list, you can enable them in either perf or the > debugfs, but they will never actually be executed. You will just get > silence even though everything appeared to be working just fine. > > I tested this out. I was not able to get any tracepoints in modules > working with the above config options. > > There's two bugs here. One, the lack of MODULE_SIG_ALL, will > make all modules non signed during the build process. That means that > all modules when loaded will be tainted as FORCED. As Mathieu stated, > you do not need the --force flag here, it just needs to see that the > kernel supports signatures but the module is not signed. In this case, > it just calls add_taint_module(), tainting the module with > FORCED_MODULE. You get a message like this: > > sunrpc: module verification failed: signature and/or required key missing - > tainting kernel > > > Now when this module adds its tracepoint, since it is marked with a > FORCE_MODULE taint flag, the tracepoint code just ignores it and does > not do any processing at all. > > Worse yet, the tracepoint code never gives any feedback if a tracepoint > was enabled or not. That is, if a tracepoint was never initialized when > the module was loaded, the tracepoint will still report success at > time of enabling, that it was registered (and tracing) even though it is > not. > > At the end of this email, I added a patch that fixes that. But I have to > concur that Mathieu did find a bug. There is no reason to disable > tracepoints in modules if someone simply has the above configs enabled > (and disabled)! > > Below is the patch that warns if the tracepoint is not enabled for > whatever reason. > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org> > > -- Steve > > > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 29f2654..b85f68f 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -62,6 +62,7 @@ struct tracepoint_entry { > struct hlist_node hlist; > struct tracepoint_func *funcs; > int refcount; /* Number of times armed. 0 if disarmed. */ > + int enabled; /* Tracepoint enabled */ > char name[0]; > }; > > @@ -237,6 +238,7 @@ static struct tracepoint_entry *add_tracepoint(const char > *name) > memcpy(&e->name[0], name, name_len); > e->funcs = NULL; > e->refcount = 0; > + e->enabled = 0; > hlist_add_head(&e->hlist, head); > return e; > } > @@ -316,6 +318,7 @@ static void tracepoint_update_probe_range(struct > tracepoint * const *begin, > if (mark_entry) { > set_tracepoint(&mark_entry, *iter, > !!mark_entry->refcount); > + mark_entry->enabled = !!mark_entry->refcount; > } else { > disable_tracepoint(*iter); > } > @@ -380,6 +383,8 @@ tracepoint_add_probe(const char *name, void *probe, void > *data) > int tracepoint_probe_register(const char *name, void *probe, void *data) > { > struct tracepoint_func *old; > + struct tracepoint_entry *entry; > + int ret = 0; > > mutex_lock(&tracepoints_mutex); > old = tracepoint_add_probe(name, probe, data); > @@ -388,9 +393,15 @@ int tracepoint_probe_register(const char *name, void > *probe, void *data) > return PTR_ERR(old); > } > tracepoint_update_probes(); /* may update entry */ > + entry = get_tracepoint(name); > + /* Make sure the entry was enabled */ > + if (!entry || !entry->enabled) { > + tracepoint_entry_remove_probe(entry, probe, data); I'm OK with returning some kind of feedback about whether the tracepoint is enabled or not, but there is one use-case I care about this change breaks: registering a tracepoint probe for a module that is not yet loaded. The change you propose here removes the probe if no tracepoint is found when the probe is registered. Another way to do this, without requiring changes to the existing tracepoint_probe_register() API, is to simply add e.g. (better function names welcome): int tracepoint_has_callsites(const char *name) { struct tracepoint_entry *entry; int ret = 0; mutex_lock(&tracepoints_mutex); entry = get_tracepoint(name); if (entry && entry->refcount) { ret = 1; } mutex_unlock(&tracepoints_mutex); return ret; } So tools which care about providing feedback to their users about the fact that tracepoint callsites are not there can call this new function. The advantage is that it would not change the return values of the existing API. Also, it would be weird to have tracepoint_probe_register() return an error value but leave the tracepoint in a registered state. Moving this into a separate query function seems more consistent IMHO. Thoughts ? Thanks, Mathieu > + ret = -ENODEV; > + } > mutex_unlock(&tracepoints_mutex); > release_probes(old); > - return 0; > + return ret; > } > EXPORT_SYMBOL_GPL(tracepoint_probe_register); > > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-14 3:49 ` Mathieu Desnoyers @ 2014-02-24 15:54 ` Steven Rostedt 2014-02-24 16:55 ` Mathieu Desnoyers 2014-02-24 18:32 ` Mathieu Desnoyers 0 siblings, 2 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-24 15:54 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Fri, 14 Feb 2014 03:49:04 +0000 (UTC) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > mutex_lock(&tracepoints_mutex); > > old = tracepoint_add_probe(name, probe, data); > > @@ -388,9 +393,15 @@ int tracepoint_probe_register(const char *name, void > > *probe, void *data) > > return PTR_ERR(old); > > } > > tracepoint_update_probes(); /* may update entry */ > > + entry = get_tracepoint(name); > > + /* Make sure the entry was enabled */ > > + if (!entry || !entry->enabled) { > > + tracepoint_entry_remove_probe(entry, probe, data); > > I'm OK with returning some kind of feedback about whether the tracepoint is > enabled or not, but there is one use-case I care about this change breaks: > registering a tracepoint probe for a module that is not yet loaded. The change > you propose here removes the probe if no tracepoint is found when the probe is > registered. The thing is, there's no in-tree user of that interface. I was thinking of adding a tracepoint_probe_register_future() that wouldn't remove the probe, but this storing of a tracepoint that does not exist (and may never exist), is IMHO a broken design. The real answer to this is to enabled the tracepoints on module load, with a module parameter. We've talked about this before, and I also think there's some patches out there that do this. (I remember creating something myself). I'll have to go dig them out and we can work to get them in 3.15. > > Another way to do this, without requiring changes to the existing > tracepoint_probe_register() API, is to simply add e.g. (better function > names welcome): > > int tracepoint_has_callsites(const char *name) > { > struct tracepoint_entry *entry; > int ret = 0; > > mutex_lock(&tracepoints_mutex); > entry = get_tracepoint(name); > if (entry && entry->refcount) { > ret = 1; > } > mutex_unlock(&tracepoints_mutex); > return ret; > } No, I'm not about to change the interface for something that is broken. tracepoint_probe_register() should fail if it does not register a tracepoint. It should not store it off for later. I'm not aware of any other "register" function in the kernel that does such a thing. Is there one that you know of? > > So tools which care about providing feedback to their users about the > fact that tracepoint callsites are not there can call this new function. > The advantage is that it would not change the return values of the existing > API. Also, it would be weird to have tracepoint_probe_register() return > an error value but leave the tracepoint in a registered state. Moving this > into a separate query function seems more consistent IMHO. Again, the existing API is not a user interface. It is free to change, and this change wont break any existing in-tree uses. In fact, the fact that you had this stupid way of doing it, *broke* the in-tree users! That is, no error messages were ever displayed when the probe was not registered. This is why I consider this a broken design. If you want LTTng to enable tracepoints before loading, then have your module save off these these tracepoints and register a handler to be called when a module is loaded and enable them yourself. For now, I'm going to push this in, and also mark it for stable. -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 15:54 ` Steven Rostedt @ 2014-02-24 16:55 ` Mathieu Desnoyers 2014-02-24 17:39 ` Steven Rostedt 2014-02-24 18:32 ` Mathieu Desnoyers 1 sibling, 1 reply; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-24 16:55 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" <dhowells@redhat.com>, > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Monday, February 24, 2014 10:54:54 AM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > [...] (keeping discussion for later, as I'm busy at a client site) > For now, I'm going to push this in, and also mark it for stable. Which patch or patches do you plan to pull, and which is marked for stable ? This thread is a RFC PATCH. I posted a separate more complete patch in a separate thread marked [PATCH]. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 16:55 ` Mathieu Desnoyers @ 2014-02-24 17:39 ` Steven Rostedt 2014-02-24 17:58 ` Mathieu Desnoyers 2014-02-26 2:53 ` Rusty Russell 0 siblings, 2 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-24 17:39 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Mon, 24 Feb 2014 16:55:36 +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: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas > > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" <dhowells@redhat.com>, > > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > > Sent: Monday, February 24, 2014 10:54:54 AM > > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > > [...] > > (keeping discussion for later, as I'm busy at a client site) > > > For now, I'm going to push this in, and also mark it for stable. > > Which patch or patches do you plan to pull, and which is marked for stable ? The one that I replied to. I can't pull the module patch unless I get an ack from Rusty. > > This thread is a RFC PATCH. I posted a separate more complete patch in > a separate thread marked [PATCH]. Yeah, I'll post it out soon enough. -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 17:39 ` Steven Rostedt @ 2014-02-24 17:58 ` Mathieu Desnoyers 2014-02-24 18:25 ` Steven Rostedt 2014-02-26 19:55 ` Steven Rostedt 2014-02-26 2:53 ` Rusty Russell 1 sibling, 2 replies; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-24 17:58 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" <dhowells@redhat.com>, > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Monday, February 24, 2014 12:39:26 PM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > On Mon, 24 Feb 2014 16:55:36 +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: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo > > > Molnar" <mingo@redhat.com>, "Thomas > > > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, > > > "David Howells" <dhowells@redhat.com>, > > > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > > > Sent: Monday, February 24, 2014 10:54:54 AM > > > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new > > > TAINT_UNSIGNED_MODULE > > > > > [...] > > > > (keeping discussion for later, as I'm busy at a client site) > > > > > For now, I'm going to push this in, and also mark it for stable. > > > > Which patch or patches do you plan to pull, and which is marked for stable > > ? > > The one that I replied to. I can't pull the module patch unless I get > an ack from Rusty. Do you mean the internal API semantic change you propose for tracepoints ? If yes, then how do you consider this a fix worthy of being backported to stable ? Thanks, Mathieu > > > > > This thread is a RFC PATCH. I posted a separate more complete patch in > > a separate thread marked [PATCH]. > > Yeah, I'll post it out soon enough. > > -- Steve > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 17:58 ` Mathieu Desnoyers @ 2014-02-24 18:25 ` Steven Rostedt 2014-02-26 19:55 ` Steven Rostedt 1 sibling, 0 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-24 18:25 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Mon, 24 Feb 2014 17:58:18 +0000 (UTC) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > The one that I replied to. I can't pull the module patch unless I get > > an ack from Rusty. > > Do you mean the internal API semantic change you propose for tracepoints ? > If yes, then how do you consider this a fix worthy of being backported to > stable ? > Actually, for now, I'm going to just add a nasty warning when a module fails to load the tracepoints. At least that should notify people that what went wrong. -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 17:58 ` Mathieu Desnoyers 2014-02-24 18:25 ` Steven Rostedt @ 2014-02-26 19:55 ` Steven Rostedt 1 sibling, 0 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-26 19:55 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Mon, 24 Feb 2014 17:58:18 +0000 (UTC) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > The one that I replied to. I can't pull the module patch unless I get > > an ack from Rusty. > > Do you mean the internal API semantic change you propose for tracepoints ? > If yes, then how do you consider this a fix worthy of being backported to > stable ? > No, I was talking about your patch. The one Rusty already said he'll pull (or I will :-) -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 17:39 ` Steven Rostedt 2014-02-24 17:58 ` Mathieu Desnoyers @ 2014-02-26 2:53 ` Rusty Russell 2014-02-26 20:13 ` Steven Rostedt 1 sibling, 1 reply; 36+ messages in thread From: Rusty Russell @ 2014-02-26 2:53 UTC (permalink / raw) To: Steven Rostedt, Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman Steven Rostedt <rostedt@goodmis.org> writes: > On Mon, 24 Feb 2014 16:55:36 +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: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas >> > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" <dhowells@redhat.com>, >> > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> >> > Sent: Monday, February 24, 2014 10:54:54 AM >> > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE >> > >> [...] >> >> (keeping discussion for later, as I'm busy at a client site) >> >> > For now, I'm going to push this in, and also mark it for stable. >> >> Which patch or patches do you plan to pull, and which is marked for stable ? > > The one that I replied to. I can't pull the module patch unless I get > an ack from Rusty. Ah, I applied it in my tree. Happy for you to take it though; here's the variant with an Acked-by from me instead of Signed-off-by if you want it: === From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Subject: Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE Users have reported being unable to trace non-signed modules loaded within a kernel supporting module signature. This is caused by tracepoint.c:tracepoint_module_coming() refusing to take into account tracepoints sitting within force-loaded modules (TAINT_FORCED_MODULE). The reason for this check, in the first place, is that a force-loaded module may have a struct module incompatible with the layout expected by the kernel, and can thus cause a kernel crash upon forced load of that module on a kernel with CONFIG_TRACEPOINTS=y. Tracepoints, however, specifically accept TAINT_OOT_MODULE and TAINT_CRAP, since those modules do not lead to the "very likely system crash" issue cited above for force-loaded modules. With kernels having CONFIG_MODULE_SIG=y (signed modules), a non-signed module is tainted re-using the TAINT_FORCED_MODULE taint flag. Unfortunately, this means that Tracepoints treat that module as a force-loaded module, and thus silently refuse to consider any tracepoint within this module. Since an unsigned module does not fit within the "very likely system crash" category of tainting, add a new TAINT_UNSIGNED_MODULE taint flag to specifically address this taint behavior, and accept those modules within Tracepoints. This flag is assigned to the letter 'N', since all the more obvious ones (e.g. 'S') were already taken. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Nacked-by: Ingo Molnar <mingo@kernel.org> CC: Steven Rostedt <rostedt@goodmis.org> CC: Thomas Gleixner <tglx@linutronix.de> CC: David Howells <dhowells@redhat.com> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Acked-by: Rusty Russell <rusty@rustcorp.com.au> --- include/linux/kernel.h | 1 + include/trace/events/module.h | 3 ++- kernel/module.c | 4 +++- kernel/panic.c | 1 + kernel/tracepoint.c | 5 +++-- 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 196d1ea86df0..471090093c67 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -469,6 +469,7 @@ extern enum system_states { #define TAINT_CRAP 10 #define TAINT_FIRMWARE_WORKAROUND 11 #define TAINT_OOT_MODULE 12 +#define TAINT_UNSIGNED_MODULE 13 extern const char hex_asc[]; #define hex_asc_lo(x) hex_asc[((x) & 0x0f)] diff --git a/include/trace/events/module.h b/include/trace/events/module.h index 161932737416..1788a02557f4 100644 --- a/include/trace/events/module.h +++ b/include/trace/events/module.h @@ -23,7 +23,8 @@ struct module; #define show_module_flags(flags) __print_flags(flags, "", \ { (1UL << TAINT_PROPRIETARY_MODULE), "P" }, \ { (1UL << TAINT_FORCED_MODULE), "F" }, \ - { (1UL << TAINT_CRAP), "C" }) + { (1UL << TAINT_CRAP), "C" }, \ + { (1UL << TAINT_UNSIGNED_MODULE), "N" }) TRACE_EVENT(module_load, diff --git a/kernel/module.c b/kernel/module.c index efa1e6031950..eadf1f1651fb 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1013,6 +1013,8 @@ static size_t module_flags_taint(struct module *mod, char *buf) buf[l++] = 'F'; if (mod->taints & (1 << TAINT_CRAP)) buf[l++] = 'C'; + if (mod->taints & (1 << TAINT_UNSIGNED_MODULE)) + buf[l++] = 'N'; /* * TAINT_FORCED_RMMOD: could be added. * TAINT_UNSAFE_SMP, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't @@ -3214,7 +3216,7 @@ static int load_module(struct load_info *info, const char __user *uargs, pr_notice_once("%s: module verification failed: signature " "and/or required key missing - tainting " "kernel\n", mod->name); - add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_STILL_OK); + add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK); } #endif diff --git a/kernel/panic.c b/kernel/panic.c index 6d6300375090..98588e0ed36a 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -210,6 +210,7 @@ static const struct tnt tnts[] = { { TAINT_CRAP, 'C', ' ' }, { TAINT_FIRMWARE_WORKAROUND, 'I', ' ' }, { TAINT_OOT_MODULE, 'O', ' ' }, + { TAINT_UNSIGNED_MODULE, 'N', ' ' }, }; /** diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index 29f26540e9c9..e7903c1ce221 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -639,9 +639,10 @@ static int tracepoint_module_coming(struct module *mod) /* * We skip modules that taint the kernel, especially those with different * module headers (for forced load), to make sure we don't cause a crash. - * Staging and out-of-tree GPL modules are fine. + * Staging, out-of-tree, and non-signed GPL modules are fine. */ - if (mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP))) + if (mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP) | + (1 << TAINT_UNSIGNED_MODULE))) return 0; mutex_lock(&tracepoints_mutex); tp_mod = kmalloc(sizeof(struct tp_module), GFP_KERNEL); ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-26 2:53 ` Rusty Russell @ 2014-02-26 20:13 ` Steven Rostedt 0 siblings, 0 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-26 20:13 UTC (permalink / raw) To: Rusty Russell Cc: Mathieu Desnoyers, Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, David Howells, Greg Kroah-Hartman On Wed, 26 Feb 2014 13:23:56 +1030 Rusty Russell <rusty@rustcorp.com.au> wrote: > > The one that I replied to. I can't pull the module patch unless I get > > an ack from Rusty. > > Ah, I applied it in my tree. Happy for you to take it though; here's > the variant with an Acked-by from me instead of Signed-off-by if you > want it: > > === > From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Subject: Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > Users have reported being unable to trace non-signed modules loaded > within a kernel supporting module signature. > > This is caused by tracepoint.c:tracepoint_module_coming() refusing to > take into account tracepoints sitting within force-loaded modules > (TAINT_FORCED_MODULE). The reason for this check, in the first place, is > that a force-loaded module may have a struct module incompatible with > the layout expected by the kernel, and can thus cause a kernel crash > upon forced load of that module on a kernel with CONFIG_TRACEPOINTS=y. > > Tracepoints, however, specifically accept TAINT_OOT_MODULE and > TAINT_CRAP, since those modules do not lead to the "very likely system > crash" issue cited above for force-loaded modules. > > With kernels having CONFIG_MODULE_SIG=y (signed modules), a non-signed > module is tainted re-using the TAINT_FORCED_MODULE taint flag. > Unfortunately, this means that Tracepoints treat that module as a > force-loaded module, and thus silently refuse to consider any tracepoint > within this module. > > Since an unsigned module does not fit within the "very likely system > crash" category of tainting, add a new TAINT_UNSIGNED_MODULE taint flag > to specifically address this taint behavior, and accept those modules > within Tracepoints. This flag is assigned to the letter 'N', since all > the more obvious ones (e.g. 'S') were already taken. > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Nacked-by: Ingo Molnar <mingo@kernel.org> > CC: Steven Rostedt <rostedt@goodmis.org> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: David Howells <dhowells@redhat.com> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Acked-by: Rusty Russell <rusty@rustcorp.com.au> There's another version of this as Mathieu pointed out. You can take it. I hate to be the committer of a patch with Ingo's Nack ;-) -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 15:54 ` Steven Rostedt 2014-02-24 16:55 ` Mathieu Desnoyers @ 2014-02-24 18:32 ` Mathieu Desnoyers 2014-02-24 19:10 ` Steven Rostedt 1 sibling, 1 reply; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-24 18:32 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" <dhowells@redhat.com>, > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Monday, February 24, 2014 10:54:54 AM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > On Fri, 14 Feb 2014 03:49:04 +0000 (UTC) > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > > > mutex_lock(&tracepoints_mutex); > > > old = tracepoint_add_probe(name, probe, data); > > > @@ -388,9 +393,15 @@ int tracepoint_probe_register(const char *name, void > > > *probe, void *data) > > > return PTR_ERR(old); > > > } > > > tracepoint_update_probes(); /* may update entry */ > > > + entry = get_tracepoint(name); > > > + /* Make sure the entry was enabled */ > > > + if (!entry || !entry->enabled) { > > > + tracepoint_entry_remove_probe(entry, probe, data); > > > > I'm OK with returning some kind of feedback about whether the tracepoint is > > enabled or not, but there is one use-case I care about this change breaks: > > registering a tracepoint probe for a module that is not yet loaded. The > > change > > you propose here removes the probe if no tracepoint is found when the probe > > is > > registered. > > The thing is, there's no in-tree user of that interface. Right. > > I was thinking of adding a tracepoint_probe_register_future() that > wouldn't remove the probe, but this storing of a tracepoint that does > not exist (and may never exist), is IMHO a broken design. It might not provide the best error reporting when probes are loaded and expect to have matching tracepoint caller sites, I agree on that part. > > The real answer to this is to enabled the tracepoints on module load, > with a module parameter. We've talked about this before, and I also > think there's some patches out there that do this. (I remember creating > something myself). I'll have to go dig them out and we can work to get > them in 3.15. For the records, I still think that requiring users of tracing to add module parameters specifying what tracing they need enabled is expecting them to interact at the wrong interface level. This might be convenient for kernel developers, but not for other types of kernel tracing end users. From a user experience perspective, I think your "real answer" is the wrong answer. > > > > > Another way to do this, without requiring changes to the existing > > tracepoint_probe_register() API, is to simply add e.g. (better function > > names welcome): > > > > int tracepoint_has_callsites(const char *name) > > { > > struct tracepoint_entry *entry; > > int ret = 0; > > > > mutex_lock(&tracepoints_mutex); > > entry = get_tracepoint(name); > > if (entry && entry->refcount) { > > ret = 1; > > } > > mutex_unlock(&tracepoints_mutex); > > return ret; > > } > > No, I'm not about to change the interface for something that is broken. > tracepoint_probe_register() should fail if it does not register a > tracepoint. It should not store it off for later. I'm not aware of any > other "register" function in the kernel that does such a thing. Is > there one that you know of? see include/linux/notifier.h You can register notifier callbacks without having any notifier call sites. This is exactly what tracepoint.c is currently doing. The change you propose is the equivalent of refusing to register a notifier callback if there is no call site for that notifier. > > > > > So tools which care about providing feedback to their users about the > > fact that tracepoint callsites are not there can call this new function. > > The advantage is that it would not change the return values of the existing > > API. Also, it would be weird to have tracepoint_probe_register() return > > an error value but leave the tracepoint in a registered state. Moving this > > into a separate query function seems more consistent IMHO. > > Again, the existing API is not a user interface. It is free to change, > and this change wont break any existing in-tree uses. In fact, the fact > that you had this stupid way of doing it, *broke* the in-tree users! > That is, no error messages were ever displayed when the probe was not > registered. This is why I consider this a broken design. > > If you want LTTng to enable tracepoints before loading, then have your > module save off these these tracepoints and register a handler to > be called when a module is loaded and enable them yourself. That's a possible option. > > For now, I'm going to push this in, and also mark it for stable. I still disagree with this change. Thanks, Mathieu > > -- Steve > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 18:32 ` Mathieu Desnoyers @ 2014-02-24 19:10 ` Steven Rostedt 2014-02-26 14:23 ` Mathieu Desnoyers 0 siblings, 1 reply; 36+ messages in thread From: Steven Rostedt @ 2014-02-24 19:10 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Mon, 24 Feb 2014 18:32:03 +0000 (UTC) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > The real answer to this is to enabled the tracepoints on module load, > > with a module parameter. We've talked about this before, and I also > > think there's some patches out there that do this. (I remember creating > > something myself). I'll have to go dig them out and we can work to get > > them in 3.15. > > For the records, I still think that requiring users of tracing to add > module parameters specifying what tracing they need enabled is expecting > them to interact at the wrong interface level. This might be convenient > for kernel developers, but not for other types of kernel tracing end > users. From a user experience perspective, I think your "real answer" > is the wrong answer. Why? This is not a normal activity to for the user. You seem to have a few specific users, but those are exceptions, and this has nothing to do with normal kernel developer view. Tracing a module that's being loaded is far from normal user activity. Now, for boot up, I could see enabling all tracepoints. For that, we can add a hook to the module load that when a flag is set, we can enable all trace events in the module as it is loaded. That would work for boot up. If this is such a user activity, please explain to me what scenario that requires tracing a module being loaded that could not easily be accomplished with a module parameter? > > > > > > > > > Another way to do this, without requiring changes to the existing > > > tracepoint_probe_register() API, is to simply add e.g. (better function > > > names welcome): > > > > > > int tracepoint_has_callsites(const char *name) > > > { > > > struct tracepoint_entry *entry; > > > int ret = 0; > > > > > > mutex_lock(&tracepoints_mutex); > > > entry = get_tracepoint(name); > > > if (entry && entry->refcount) { > > > ret = 1; > > > } > > > mutex_unlock(&tracepoints_mutex); > > > return ret; > > > } > > > > No, I'm not about to change the interface for something that is broken. > > tracepoint_probe_register() should fail if it does not register a > > tracepoint. It should not store it off for later. I'm not aware of any > > other "register" function in the kernel that does such a thing. Is > > there one that you know of? > > see include/linux/notifier.h > > You can register notifier callbacks without having any notifier call sites. > This is exactly what tracepoint.c is currently doing. The change you propose > is the equivalent of refusing to register a notifier callback if there is > no call site for that notifier. WTF! That's a horrible example. Yes, notifier is the infrastructure code (a header file), but where is there a registered list without callback sites? Look at include/linux/module.h! Do we allow to register module notifiers when modules are not configured in? NO! The tracepoint code does much more than registering a notifier. It *enables* the tracepoint. I'll reverse your example on you. If you call a notifier that has nothing registered, it does the same work that it would do if something was registered to it. But the loop is empty, it just doesn't call anything. The tracepoint code does the work to activate the call site. Here's where your analogy is broken. If I register a handler for a notifier, when the call site is hit, my handler will be called. Well, because of not doing anything different for non existent modules and modules that fail to have their call sites activated, the register returns success in both cases. That means, if I register a probe, it wont be called at the call site. That's a big f'ing bug. > > > > > > > > > So tools which care about providing feedback to their users about the > > > fact that tracepoint callsites are not there can call this new function. > > > The advantage is that it would not change the return values of the existing > > > API. Also, it would be weird to have tracepoint_probe_register() return > > > an error value but leave the tracepoint in a registered state. Moving this > > > into a separate query function seems more consistent IMHO. > > > > Again, the existing API is not a user interface. It is free to change, > > and this change wont break any existing in-tree uses. In fact, the fact > > that you had this stupid way of doing it, *broke* the in-tree users! > > That is, no error messages were ever displayed when the probe was not > > registered. This is why I consider this a broken design. > > > > If you want LTTng to enable tracepoints before loading, then have your > > module save off these these tracepoints and register a handler to > > be called when a module is loaded and enable them yourself. > > That's a possible option. > > > > > For now, I'm going to push this in, and also mark it for stable. > > I still disagree with this change. Of course you do ;-) -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-24 19:10 ` Steven Rostedt @ 2014-02-26 14:23 ` Mathieu Desnoyers 2014-02-26 15:05 ` Steven Rostedt 0 siblings, 1 reply; 36+ messages in thread From: Mathieu Desnoyers @ 2014-02-26 14:23 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman ----- Original Message ----- > From: "Steven Rostedt" <rostedt@goodmis.org> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Ingo Molnar" <mingo@kernel.org>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@redhat.com>, "Thomas > Gleixner" <tglx@linutronix.de>, "Rusty Russell" <rusty@rustcorp.com.au>, "David Howells" <dhowells@redhat.com>, > "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> > Sent: Monday, February 24, 2014 2:10:07 PM > Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE > > On Mon, 24 Feb 2014 18:32:03 +0000 (UTC) > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > > > > > The real answer to this is to enabled the tracepoints on module load, > > > with a module parameter. We've talked about this before, and I also > > > think there's some patches out there that do this. (I remember creating > > > something myself). I'll have to go dig them out and we can work to get > > > them in 3.15. > > > > For the records, I still think that requiring users of tracing to add > > module parameters specifying what tracing they need enabled is expecting > > them to interact at the wrong interface level. This might be convenient > > for kernel developers, but not for other types of kernel tracing end > > users. From a user experience perspective, I think your "real answer" > > is the wrong answer. > > > Why? This is not a normal activity to for the user. You seem to have a > few specific users, but those are exceptions, and this has nothing to > do with normal kernel developer view. The very fact that you present "normal kernel developer view" as being the norm just tells how much we focus on different user bases. Since the user-base you target are mostly kernel developers, it is understandable that you dismiss our user base as being small and specific. There are many more Linux users than kernel developers out there. The main difference is that the former category don't usually post on LKML. > > Tracing a module that's being loaded is far from normal user activity. > > Now, for boot up, I could see enabling all tracepoints. For that, we > can add a hook to the module load that when a flag is set, we can > enable all trace events in the module as it is loaded. That would work > for boot up. > > If this is such a user activity, please explain to me what scenario > that requires tracing a module being loaded that could not easily be > accomplished with a module parameter? OK. Here is the scenario: - We have users deploying tracing on their systems in flight recorder "snapshot" mode (writing into circular memory buffers, without any other type of I/O, except when a snapshot of the buffers is needed). They wish collect data from user-space and kernel-space, including from kernel modules. You can think of it like a detailed crash tracing for Linux distributions with very low overhead. Which tracepoints are enabled depends on configuration of this flight recorder tracing "session". There may be multiple concurrent tracing sessions enabled in parallel, trying each to pinpoint different kind of issues. Some are controlled by the distro end-user, others are automatically enabled by the distro if the user agrees to provide detailed bug report feedback to the distro. - There, an automated analysis wants to hook on specific module tracepoints (e.g. the usb stack) which are loaded only when the devices are physically plugged into the computer. It would not be appropriate to change a global state (whether the tracepoint is enabled or not for this module) for something specific to a subset of the tracing sessions. Moreover, you cannot expect an issue-monitoring tool within the distribution to start modifying the module parameters across the entire system. Chances are that it will conflict with other tools and user-specific configuration if it try to do so. > > > > > > > > > > > > > > Another way to do this, without requiring changes to the existing > > > > tracepoint_probe_register() API, is to simply add e.g. (better function > > > > names welcome): > > > > > > > > int tracepoint_has_callsites(const char *name) > > > > { > > > > struct tracepoint_entry *entry; > > > > int ret = 0; > > > > > > > > mutex_lock(&tracepoints_mutex); > > > > entry = get_tracepoint(name); > > > > if (entry && entry->refcount) { > > > > ret = 1; > > > > } > > > > mutex_unlock(&tracepoints_mutex); > > > > return ret; > > > > } > > > > > > No, I'm not about to change the interface for something that is broken. > > > tracepoint_probe_register() should fail if it does not register a > > > tracepoint. It should not store it off for later. I'm not aware of any > > > other "register" function in the kernel that does such a thing. Is > > > there one that you know of? > > > > see include/linux/notifier.h > > > > You can register notifier callbacks without having any notifier call sites. > > This is exactly what tracepoint.c is currently doing. The change you > > propose > > is the equivalent of refusing to register a notifier callback if there is > > no call site for that notifier. > > WTF! That's a horrible example. Yes, notifier is the infrastructure > code (a header file), but where is there a registered list without > callback sites? Look at include/linux/module.h! Do we allow to register > module notifiers when modules are not configured in? NO! See drivers/acpi/events.c: acpi_notifier_call_chain() This notifier chain is defined within events.c, compiled whenever ACPI is configured in the kernel (CONFIG_ACPI). All of its callers are: drivers/acpi/video.c: depends on CONFIG_ACPI_AC acpi/ac.c: depends on CONFIG_ACPI_VIDEO So yes, there are cases where the notifier block head is there and the modules calling into it are not loaded. > > The tracepoint code does much more than registering a notifier. It > *enables* the tracepoint. Per callsite tracepoint activation is merely an optimization over a textbook callback mechanism. We want to skip the empty loop very quickly, hence we skip over it with a conditional branch, or static jump (if available). The tracepoints know callsites registered from the core kernel and at module load. Tracepoints also register probes for those callsites. Whenever a registered probe matches a registered callsite, it is added to the list of callbacks for that callsite. If there is at least one probe in a callsite callback list, the callsite is enabled. > I'll reverse your example on you. If you call > a notifier that has nothing registered, it does the same work that it > would do if something was registered to it. But the loop is empty, it > just doesn't call anything. A disabled tracepoint is just like an empty loop. The only difference is that it is optimized for the common case: empty callback list. > > The tracepoint code does the work to activate the call site. Here's > where your analogy is broken. If I register a handler for a notifier, > when the call site is hit, my handler will be called. Well, because of > not doing anything different for non existent modules and modules that > fail to have their call sites activated, the register returns success > in both cases. That means, if I register a probe, it wont be called at > the call site. That's a big f'ing bug. If the probe and the callsite match, Tracepoints guarantee that the probe is added to the module tracepoint callsite. The bug here is that tracepoint.c silently ignored certain classes of tainted modules without giving any error message to the user. I think your approach of logging an error whenever tracepoints decide to disregard force-loaded modules is a good approach to at least tell users there is something going wrong. Splitting the "unsigned module" case from "force loaded" module is going to let users have tracepoints behaving as expected in unsigned modules within signed kernels, which I think is the second step in making everything right. Thanks, Mathieu > > > > > > > > > > > > > > > So tools which care about providing feedback to their users about the > > > > fact that tracepoint callsites are not there can call this new > > > > function. > > > > The advantage is that it would not change the return values of the > > > > existing > > > > API. Also, it would be weird to have tracepoint_probe_register() return > > > > an error value but leave the tracepoint in a registered state. Moving > > > > this > > > > into a separate query function seems more consistent IMHO. > > > > > > Again, the existing API is not a user interface. It is free to change, > > > and this change wont break any existing in-tree uses. In fact, the fact > > > that you had this stupid way of doing it, *broke* the in-tree users! > > > That is, no error messages were ever displayed when the probe was not > > > registered. This is why I consider this a broken design. > > > > > > If you want LTTng to enable tracepoints before loading, then have your > > > module save off these these tracepoints and register a handler to > > > be called when a module is loaded and enable them yourself. > > > > That's a possible option. > > > > > > > > For now, I'm going to push this in, and also mark it for stable. > > > > I still disagree with this change. > > Of course you do ;-) > > -- Steve > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE 2014-02-26 14:23 ` Mathieu Desnoyers @ 2014-02-26 15:05 ` Steven Rostedt 0 siblings, 0 replies; 36+ messages in thread From: Steven Rostedt @ 2014-02-26 15:05 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Ingo Molnar, linux-kernel, Ingo Molnar, Thomas Gleixner, Rusty Russell, David Howells, Greg Kroah-Hartman On Wed, 26 Feb 2014 14:23:22 +0000 (UTC) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > Why? This is not a normal activity to for the user. You seem to have a > > few specific users, but those are exceptions, and this has nothing to > > do with normal kernel developer view. > > The very fact that you present "normal kernel developer view" as being > the norm just tells how much we focus on different user bases. Since > the user-base you target are mostly kernel developers, it is > understandable that you dismiss our user base as being small and > specific. > > There are many more Linux users than kernel developers out there. The > main difference is that the former category don't usually post on LKML. > Yes, there are many more Linux users than kernel developers, but for those that need to look at tracepoints (the internals of the happenings of the kernel), of the tracepoint users, there are many more kernel developers than Linux users. That is key here in this debate! > > > > Tracing a module that's being loaded is far from normal user activity. > > > > Now, for boot up, I could see enabling all tracepoints. For that, we > > can add a hook to the module load that when a flag is set, we can > > enable all trace events in the module as it is loaded. That would work > > for boot up. > > > > If this is such a user activity, please explain to me what scenario > > that requires tracing a module being loaded that could not easily be > > accomplished with a module parameter? > > OK. Here is the scenario: > > - We have users deploying tracing on their systems in flight recorder Yes you have users, but these are a very minority of Linux users. > "snapshot" mode (writing into circular memory buffers, without any > other type of I/O, except when a snapshot of the buffers is needed). > They wish collect data from user-space and kernel-space, including > from kernel modules. You can think of it like a detailed crash And here is the key difference again. They want to know detail information on the happenings of the kernel. At this point, they cease from being normal users, and are boarding with kernel developers. > tracing for Linux distributions with very low overhead. Which > tracepoints are enabled depends on configuration of this flight > recorder tracing "session". There may be multiple concurrent tracing > sessions enabled in parallel, trying each to pinpoint different kind > of issues. Some are controlled by the distro end-user, others are > automatically enabled by the distro if the user agrees to provide > detailed bug report feedback to the distro. Again, this is all a very small niche, and not one that we need to bend over backwards for (ie, not warning about tracepoints not being enabled). > - There, an automated analysis wants to hook on specific module > tracepoints (e.g. the usb stack) which are loaded only when the > devices are physically plugged into the computer. Again, look at what you are saying. "hook on a specific module". THIS IS NOT A NORMAL LINUX USER. This is a very small niche, and those that do such a thing, are more like those that may become or are kernel developers. If a company is doing this, then they can afford to do the work arounds that are required (ie, module parameters), and not change the policy of the kernel internals. > > It would not be appropriate to change a global state (whether the > tracepoint is enabled or not for this module) for something specific > to a subset of the tracing sessions. Moreover, you cannot expect an > issue-monitoring tool within the distribution to start modifying the > module parameters across the entire system. Chances are that it will > conflict with other tools and user-specific configuration if it try > to do so. These are your tools that require out of tree modules. Because, there's no user app that uses the callbacks of tracepoints directly. I'm sure it wouldn't be hard to have your module do this enabling on module load, with a module callback. > > WTF! That's a horrible example. Yes, notifier is the infrastructure > > code (a header file), but where is there a registered list without > > callback sites? Look at include/linux/module.h! Do we allow to register > > module notifiers when modules are not configured in? NO! > > See drivers/acpi/events.c: acpi_notifier_call_chain() > > This notifier chain is defined within events.c, compiled whenever > ACPI is configured in the kernel (CONFIG_ACPI). All of its callers > are: > > drivers/acpi/video.c: depends on CONFIG_ACPI_AC > acpi/ac.c: depends on CONFIG_ACPI_VIDEO > > So yes, there are cases where the notifier block head is there and the > modules calling into it are not loaded. WTF? This is completely different. All you are stating is that there's some dead code here. There's no users of it. Where as, what I'm bitching about is the fact that we have users and things are not working because of this crap. > > > > > The tracepoint code does much more than registering a notifier. It > > *enables* the tracepoint. > > Per callsite tracepoint activation is merely an optimization over a > textbook callback mechanism. We want to skip the empty loop very quickly, > hence we skip over it with a conditional branch, or static jump (if > available). It still enables it, even without jump labels. > > The tracepoints know callsites registered from the core kernel and at > module load. Tracepoints also register probes for those callsites. > Whenever a registered probe matches a registered callsite, it is added > to the list of callbacks for that callsite. If there is at least one > probe in a callsite callback list, the callsite is enabled. Right, but because of this hack, nothing will ever get registered to it. > > > I'll reverse your example on you. If you call > > a notifier that has nothing registered, it does the same work that it > > would do if something was registered to it. But the loop is empty, it > > just doesn't call anything. > > A disabled tracepoint is just like an empty loop. The only difference > is that it is optimized for the common case: empty callback list. Right. But this isn't what I'm bitching about. We say "register_this_callback", and NOTHING HAPPENS!!!! THAT'S THE BUG! The reason this bug exists, is that something went wrong with the tracepoint, and it doesn't "exist". But when we go to enable it, we don't get a warning that it doesn't exist. It just says "OK, we enabled it". BUT IT DID NOT DO ANYTHING. IT LIED! > > > > > The tracepoint code does the work to activate the call site. Here's > > where your analogy is broken. If I register a handler for a notifier, > > when the call site is hit, my handler will be called. Well, because of > > not doing anything different for non existent modules and modules that > > fail to have their call sites activated, the register returns success > > in both cases. That means, if I register a probe, it wont be called at > > the call site. That's a big f'ing bug. > > If the probe and the callsite match, Tracepoints guarantee that the probe > is added to the module tracepoint callsite. The bug here is that > tracepoint.c silently ignored certain classes of tainted modules without > giving any error message to the user. I think your approach of logging > an error whenever tracepoints decide to disregard force-loaded modules > is a good approach to at least tell users there is something going > wrong. Splitting the "unsigned module" case from "force loaded" module > is going to let users have tracepoints behaving as expected in unsigned > modules within signed kernels, which I think is the second step in making > everything right. The thing is, if people don't see this error, it causes confusion elsewhere. -- Steve ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2014-02-26 20:13 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-02-10 23:23 [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE Mathieu Desnoyers 2014-02-11 7:27 ` Ingo Molnar 2014-02-12 4:45 ` Steven Rostedt 2014-02-12 5:51 ` Mathieu Desnoyers 2014-02-13 3:24 ` Rusty Russell 2014-02-13 21:11 ` Steven Rostedt 2014-02-13 21:24 ` Steven Rostedt 2014-02-14 3:32 ` Mathieu Desnoyers 2014-02-14 0:51 ` Rusty Russell 2014-02-16 23:58 ` Mathieu Desnoyers 2014-02-20 15:30 ` Steven Rostedt 2014-02-20 23:09 ` Rusty Russell 2014-02-21 4:09 ` Steven Rostedt 2014-02-21 8:10 ` Johannes Berg 2014-02-26 2:51 ` Rusty Russell 2014-02-26 12:55 ` Mathieu Desnoyers 2014-02-13 15:10 ` Mathieu Desnoyers 2014-02-13 15:28 ` Steven Rostedt 2014-02-13 15:36 ` Frank Ch. Eigler 2014-02-13 15:44 ` Steven Rostedt 2014-02-13 21:42 ` Arend van Spriel 2014-02-13 15:41 ` Mathieu Desnoyers 2014-02-13 20:45 ` Steven Rostedt 2014-02-14 3:49 ` Mathieu Desnoyers 2014-02-24 15:54 ` Steven Rostedt 2014-02-24 16:55 ` Mathieu Desnoyers 2014-02-24 17:39 ` Steven Rostedt 2014-02-24 17:58 ` Mathieu Desnoyers 2014-02-24 18:25 ` Steven Rostedt 2014-02-26 19:55 ` Steven Rostedt 2014-02-26 2:53 ` Rusty Russell 2014-02-26 20:13 ` Steven Rostedt 2014-02-24 18:32 ` Mathieu Desnoyers 2014-02-24 19:10 ` Steven Rostedt 2014-02-26 14:23 ` Mathieu Desnoyers 2014-02-26 15:05 ` Steven Rostedt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.