Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Wed, 26 Feb 2014 13:23:56 +1030 Rusty Russell 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 > 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 > Nacked-by: Ingo Molnar > CC: Steven Rostedt > CC: Thomas Gleixner > CC: David Howells > CC: Greg Kroah-Hartman > Acked-by: Rusty Russell 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Mon, 24 Feb 2014 17:58:18 + (UTC) Mathieu Desnoyers 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Wed, 26 Feb 2014 14:23:22 + (UTC) Mathieu Desnoyers 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
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message -
> From: "Steven Rostedt"
> To: "Mathieu Desnoyers"
> Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo
> Molnar" , "Thomas
> Gleixner" , "Rusty Russell" ,
> "David Howells" ,
> "Greg Kroah-Hartman"
> 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 + (UTC)
> Mathieu Desnoyers 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(_mutex);
> > > > entry = get_tracepoint(name);
> > > > if (entry && entry->refcount) {
> > > > ret = 1;
> > > > }
> > > > mutex_unlock(_mutex);
> > > > return ret;
> > > > }
> > >
> > > No, I'm not about to change the interface for something that is broken.
> > > tracepoint_probe_r
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - > From: "Rusty Russell" > To: "Johannes Berg" , "Steven Rostedt" > > Cc: "Ingo Molnar" , "Mathieu Desnoyers" > , > linux-kernel@vger.kernel.org, "Ingo Molnar" , "Thomas > Gleixner" , "David > Howells" , "Greg Kroah-Hartman" > > 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - From: Rusty Russell ru...@rustcorp.com.au To: Johannes Berg johan...@sipsolutions.net, Steven Rostedt rost...@goodmis.org Cc: Ingo Molnar mi...@kernel.org, Mathieu Desnoyers mathieu.desnoy...@efficios.com, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 johan...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message -
From: Steven Rostedt rost...@goodmis.org
To: Mathieu Desnoyers mathieu.desnoy...@efficios.com
Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo
Molnar mi...@redhat.com, Thomas
Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au,
David Howells dhowe...@redhat.com,
Greg Kroah-Hartman gre...@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 + (UTC)
Mathieu Desnoyers mathieu.desnoy...@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
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Wed, 26 Feb 2014 14:23:22 + (UTC) Mathieu Desnoyers mathieu.desnoy...@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
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Mon, 24 Feb 2014 17:58:18 + (UTC) Mathieu Desnoyers mathieu.desnoy...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Wed, 26 Feb 2014 13:23:56 +1030 Rusty Russell ru...@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.desnoy...@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.desnoy...@efficios.com Nacked-by: Ingo Molnar mi...@kernel.org CC: Steven Rostedt rost...@goodmis.org CC: Thomas Gleixner t...@linutronix.de CC: David Howells dhowe...@redhat.com CC: Greg Kroah-Hartman gre...@linuxfoundation.org Acked-by: Rusty Russell ru...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Steven Rostedt writes:
> On Mon, 24 Feb 2014 16:55:36 + (UTC)
> Mathieu Desnoyers wrote:
>
>> - Original Message -
>> > From: "Steven Rostedt"
>> > To: "Mathieu Desnoyers"
>> > Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo
>> > Molnar" , "Thomas
>> > Gleixner" , "Rusty Russell" ,
>> > "David Howells" ,
>> > "Greg Kroah-Hartman"
>> > 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
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
Nacked-by: Ingo Molnar
CC: Steven Rostedt
CC: Thomas Gleixner
CC: David Howells
CC: Greg Kroah-Hartman
Acked-by: Rusty Russell
---
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,
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Johannes Berg 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Johannes Berg johan...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Steven Rostedt rost...@goodmis.org writes:
On Mon, 24 Feb 2014 16:55:36 + (UTC)
Mathieu Desnoyers mathieu.desnoy...@efficios.com wrote:
- Original Message -
From: Steven Rostedt rost...@goodmis.org
To: Mathieu Desnoyers mathieu.desnoy...@efficios.com
Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo
Molnar mi...@redhat.com, Thomas
Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au,
David Howells dhowe...@redhat.com,
Greg Kroah-Hartman gre...@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.desnoy...@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.desnoy...@efficios.com
Nacked-by: Ingo Molnar mi...@kernel.org
CC: Steven Rostedt rost...@goodmis.org
CC: Thomas Gleixner t...@linutronix.de
CC: David Howells dhowe...@redhat.com
CC: Greg Kroah-Hartman gre...@linuxfoundation.org
Acked-by: Rusty Russell ru...@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
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Mon, 24 Feb 2014 18:32:03 + (UTC)
Mathieu Desnoyers 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(_mutex);
> > > entry = get_tracepoint(name);
> > > if (entry && entry->refcount) {
> > > ret = 1;
> > > }
> > > mutex_unlock(_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message -
> From: "Steven Rostedt"
> To: "Mathieu Desnoyers"
> Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo
> Molnar" , "Thomas
> Gleixner" , "Rusty Russell" ,
> "David Howells" ,
> "Greg Kroah-Hartman"
> 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 + (UTC)
> Mathieu Desnoyers wrote:
> > >
> > > mutex_lock(_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(_mutex);
> > entry = get_tracepoint(name);
> > if (entry && entry->refcount) {
> > ret = 1;
> > }
> > mutex_unlock(_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 tracepoin
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Mon, 24 Feb 2014 17:58:18 + (UTC) Mathieu Desnoyers 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - > From: "Steven Rostedt" > To: "Mathieu Desnoyers" > Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo > Molnar" , "Thomas > Gleixner" , "Rusty Russell" , > "David Howells" , > "Greg Kroah-Hartman" > 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 + (UTC) > Mathieu Desnoyers wrote: > > > - Original Message - > > > From: "Steven Rostedt" > > > To: "Mathieu Desnoyers" > > > Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo > > > Molnar" , "Thomas > > > Gleixner" , "Rusty Russell" , > > > "David Howells" , > > > "Greg Kroah-Hartman" > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Mon, 24 Feb 2014 16:55:36 + (UTC) Mathieu Desnoyers wrote: > - Original Message - > > From: "Steven Rostedt" > > To: "Mathieu Desnoyers" > > Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo > > Molnar" , "Thomas > > Gleixner" , "Rusty Russell" , > > "David Howells" , > > "Greg Kroah-Hartman" > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - > From: "Steven Rostedt" > To: "Mathieu Desnoyers" > Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo > Molnar" , "Thomas > Gleixner" , "Rusty Russell" , > "David Howells" , > "Greg Kroah-Hartman" > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Fri, 14 Feb 2014 03:49:04 + (UTC)
Mathieu Desnoyers wrote:
> >
> > mutex_lock(_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(_mutex);
> entry = get_tracepoint(name);
> if (entry && entry->refcount) {
> ret = 1;
> }
> mutex_unlock(_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Fri, 14 Feb 2014 03:49:04 + (UTC)
Mathieu Desnoyers mathieu.desnoy...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - From: Steven Rostedt rost...@goodmis.org To: Mathieu Desnoyers mathieu.desnoy...@efficios.com Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Mon, 24 Feb 2014 16:55:36 + (UTC) Mathieu Desnoyers mathieu.desnoy...@efficios.com wrote: - Original Message - From: Steven Rostedt rost...@goodmis.org To: Mathieu Desnoyers mathieu.desnoy...@efficios.com Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - From: Steven Rostedt rost...@goodmis.org To: Mathieu Desnoyers mathieu.desnoy...@efficios.com Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 + (UTC) Mathieu Desnoyers mathieu.desnoy...@efficios.com wrote: - Original Message - From: Steven Rostedt rost...@goodmis.org To: Mathieu Desnoyers mathieu.desnoy...@efficios.com Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Mon, 24 Feb 2014 17:58:18 + (UTC) Mathieu Desnoyers mathieu.desnoy...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message -
From: Steven Rostedt rost...@goodmis.org
To: Mathieu Desnoyers mathieu.desnoy...@efficios.com
Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo
Molnar mi...@redhat.com, Thomas
Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au,
David Howells dhowe...@redhat.com,
Greg Kroah-Hartman gre...@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 + (UTC)
Mathieu Desnoyers mathieu.desnoy...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Mon, 24 Feb 2014 18:32:03 + (UTC)
Mathieu Desnoyers mathieu.desnoy...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 2014-02-20 at 23:09 -0500, Steven Rostedt wrote: > On Fri, 21 Feb 2014 09:39:18 +1030 > Rusty Russell 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 2014-02-20 at 23:09 -0500, Steven Rostedt wrote: On Fri, 21 Feb 2014 09:39:18 +1030 Rusty Russell ru...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Fri, 21 Feb 2014 09:39:18 +1030 Rusty Russell 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Steven Rostedt 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 wrote: > >> Steven Rostedt writes: >> > On Thu, 13 Feb 2014 13:54:42 +1030 >> > Rusty Russell 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
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 wrote: > Steven Rostedt writes: > > On Thu, 13 Feb 2014 13:54:42 +1030 > > Rusty Russell 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
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 ru...@rustcorp.com.au wrote: Steven Rostedt rost...@goodmis.org writes: On Thu, 13 Feb 2014 13:54:42 +1030 Rusty Russell ru...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Steven Rostedt rost...@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 ru...@rustcorp.com.au wrote: Steven Rostedt rost...@goodmis.org writes: On Thu, 13 Feb 2014 13:54:42 +1030 Rusty Russell ru...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Fri, 21 Feb 2014 09:39:18 +1030 Rusty Russell ru...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - > From: "Rusty Russell" > To: "Steven Rostedt" > Cc: "Ingo Molnar" , "Mathieu Desnoyers" > , > linux-kernel@vger.kernel.org, "Ingo Molnar" , "Thomas > Gleixner" , "David > Howells" , "Greg Kroah-Hartman" > > 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 writes: > > On Thu, 13 Feb 2014 13:54:42 +1030 > > Rusty Russell 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Steven Rostedt writes: > On Thu, 13 Feb 2014 13:54:42 +1030 > Rusty Russell 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Steven Rostedt rost...@goodmis.org writes: On Thu, 13 Feb 2014 13:54:42 +1030 Rusty Russell ru...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - From: Rusty Russell ru...@rustcorp.com.au To: Steven Rostedt rost...@goodmis.org Cc: Ingo Molnar mi...@kernel.org, Mathieu Desnoyers mathieu.desnoy...@efficios.com, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 rost...@goodmis.org writes: On Thu, 13 Feb 2014 13:54:42 +1030 Rusty Russell ru...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message -
> From: "Steven Rostedt"
> To: "Mathieu Desnoyers"
> Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo
> Molnar" , "Thomas
> Gleixner" , "Rusty Russell" ,
> "David Howells" ,
> "Greg Kroah-Hartman"
> 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 + (UTC)
> Mathieu Desnoyers 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
>
> -- 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(>name[0], name, name_len);
> e->funcs = NULL;
> e->refcount = 0;
> + e->enabled = 0;
> hlist_add_head(>hlist, head);
> return e;
> }
> @@ -316,6 +318,7 @@ static void tracepoint_update_probe_range(struct
> tracepoint * const *begin,
> if (mark_entry) {
> set_tracepoint(_entry, *iter,
> !
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - > From: "Steven Rostedt" > To: "Steven Rostedt" > Cc: "Rusty Russell" , "Ingo Molnar" > , "Mathieu Desnoyers" > , linux-kernel@vger.kernel.org, "Ingo Molnar" > , "Thomas Gleixner" > , "David Howells" , "Greg > Kroah-Hartman" > 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On 02/13/2014 04:44 PM, Steven Rostedt wrote: > On Thu, 13 Feb 2014 10:36:35 -0500 > f...@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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 16:11:56 -0500 Steven Rostedt 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 13:54:42 +1030
Rusty Russell 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 15:41:30 + (UTC)
Mathieu Desnoyers 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
-- 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(>name[0], name, name_len);
e->funcs = NULL;
e->refcount = 0;
+ e->enabled = 0;
hlist_add_head(>hlist, head);
return e;
}
@@ -316,6 +318,7 @@ static void tracepoint_update_probe_range(struct tracepoint
* const *begin,
if (mark_entry) {
set_tracepoint(_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(_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(_mutex);
release_probes(old);
- return 0;
+ return
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 10:36:35 -0500 f...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - > From: "Steven Rostedt" > To: "Mathieu Desnoyers" > Cc: "Ingo Molnar" , linux-kernel@vger.kernel.org, "Ingo > Molnar" , "Thomas > Gleixner" , "Rusty Russell" , > "David Howells" , > "Greg Kroah-Hartman" > 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 + (UTC) > Mathieu Desnoyers wrote: > > > - Original Message - > > > From: "Steven Rostedt" > > > To: "Ingo Molnar" > > > Cc: "Mathieu Desnoyers" , > > > linux-kernel@vger.kernel.org, "Ingo Molnar" > > > , "Thomas Gleixner" , "Rusty > > > Russell" , "David Howells" > > > , "Greg Kroah-Hartman" > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 15:10:14 + (UTC) Mathieu Desnoyers wrote: > - Original Message - > > From: "Steven Rostedt" > > To: "Ingo Molnar" > > Cc: "Mathieu Desnoyers" , > > linux-kernel@vger.kernel.org, "Ingo Molnar" > > , "Thomas Gleixner" , "Rusty Russell" > > , "David Howells" > > , "Greg Kroah-Hartman" > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - > From: "Steven Rostedt" > To: "Ingo Molnar" > Cc: "Mathieu Desnoyers" , > linux-kernel@vger.kernel.org, "Ingo Molnar" > , "Thomas Gleixner" , "Rusty Russell" > , "David Howells" > , "Greg Kroah-Hartman" > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - From: Steven Rostedt rost...@goodmis.org To: Ingo Molnar mi...@kernel.org Cc: Mathieu Desnoyers mathieu.desnoy...@efficios.com, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 15:10:14 + (UTC) Mathieu Desnoyers mathieu.desnoy...@efficios.com wrote: - Original Message - From: Steven Rostedt rost...@goodmis.org To: Ingo Molnar mi...@kernel.org Cc: Mathieu Desnoyers mathieu.desnoy...@efficios.com, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - From: Steven Rostedt rost...@goodmis.org To: Mathieu Desnoyers mathieu.desnoy...@efficios.com Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 + (UTC) Mathieu Desnoyers mathieu.desnoy...@efficios.com wrote: - Original Message - From: Steven Rostedt rost...@goodmis.org To: Ingo Molnar mi...@kernel.org Cc: Mathieu Desnoyers mathieu.desnoy...@efficios.com, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 10:36:35 -0500 f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 15:41:30 + (UTC)
Mathieu Desnoyers mathieu.desnoy...@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 rost...@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);
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 13:54:42 +1030
Rusty Russell ru...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Thu, 13 Feb 2014 16:11:56 -0500 Steven Rostedt rost...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On 02/13/2014 04:44 PM, Steven Rostedt wrote: On Thu, 13 Feb 2014 10:36:35 -0500 f...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - From: Steven Rostedt rost...@goodmis.org To: Steven Rostedt rost...@goodmis.org Cc: Rusty Russell ru...@rustcorp.com.au, Ingo Molnar mi...@kernel.org, Mathieu Desnoyers mathieu.desnoy...@efficios.com, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 rost...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message -
From: Steven Rostedt rost...@goodmis.org
To: Mathieu Desnoyers mathieu.desnoy...@efficios.com
Cc: Ingo Molnar mi...@kernel.org, linux-kernel@vger.kernel.org, Ingo
Molnar mi...@redhat.com, Thomas
Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au,
David Howells dhowe...@redhat.com,
Greg Kroah-Hartman gre...@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 + (UTC)
Mathieu Desnoyers mathieu.desnoy...@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 rost...@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
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Steven Rostedt writes: > On Tue, 11 Feb 2014 08:27:38 +0100 > Ingo Molnar wrote: > >> >> * Mathieu Desnoyers 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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
Steven Rostedt rost...@goodmis.org writes: On Tue, 11 Feb 2014 08:27:38 +0100 Ingo Molnar mi...@kernel.org wrote: * Mathieu Desnoyers mathieu.desnoy...@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 mi...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - > From: "Steven Rostedt" > To: "Ingo Molnar" > Cc: "Mathieu Desnoyers" , > linux-kernel@vger.kernel.org, "Ingo Molnar" > , "Thomas Gleixner" , "Rusty Russell" > , "David Howells" > , "Greg Kroah-Hartman" > 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 wrote: > > > > > * Mathieu Desnoyers 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 > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Tue, 11 Feb 2014 08:27:38 +0100 Ingo Molnar wrote: > > * Mathieu Desnoyers 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
On Tue, 11 Feb 2014 08:27:38 +0100 Ingo Molnar mi...@kernel.org wrote: * Mathieu Desnoyers mathieu.desnoy...@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 mi...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
- Original Message - From: Steven Rostedt rost...@goodmis.org To: Ingo Molnar mi...@kernel.org Cc: Mathieu Desnoyers mathieu.desnoy...@efficios.com, linux-kernel@vger.kernel.org, Ingo Molnar mi...@redhat.com, Thomas Gleixner t...@linutronix.de, Rusty Russell ru...@rustcorp.com.au, David Howells dhowe...@redhat.com, Greg Kroah-Hartman gre...@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 mi...@kernel.org wrote: * Mathieu Desnoyers mathieu.desnoy...@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 mi...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
* Mathieu Desnoyers 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 Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
* Mathieu Desnoyers mathieu.desnoy...@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 mi...@kernel.org Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
