CONFIG_MARKERS

CONFIG_MARKERS

[Jon Masters]

Yo,

I notice in module.c:

#ifdef CONFIG_MARKERS
	if (!mod->taints)
		marker_update_probe_range(mod->markers,
			mod->markers + mod->num_markers, NULL, NULL);
#endif

Is this an attempt to not set a marker for proprietary modules? If so,
then this really should be the following conditional instead, because,
really we're not guaranteeing there won't be other taints (e.g. in RHEL
we already have the module signing patch, and then there's also the
TAINT_FORCED_MODULE, which arguably isn't a "taint" for markers):

#ifdef CONFIG_MARKERS
	if (!(mod->taints & TAINT_PROPRIETARY_MODULE))
		marker_update_probe_range(mod->markers,
			mod->markers + mod->num_markers, NULL, NULL);
#endif

Or am I missing something?

Jon.

Re: CONFIG_MARKERS

[Frank Ch. Eigler]

Jon Masters <jcm@redhat.com> writes:

> I notice in module.c:
>
> #ifdef CONFIG_MARKERS
> 	if (!mod->taints)
> 		marker_update_probe_range(mod->markers,
> 			mod->markers + mod->num_markers, NULL, NULL);
> #endif
>
> Is this an attempt to not set a marker for proprietary modules? [...]

I can't seem to find any discussion about this aspect.  If this is the
intent, it seems misguided to me.  There may instead be a relationship
to TAINT_FORCED_{RMMOD,MODULE}.  Mathieu?

- FChE

Re: CONFIG_MARKERS

[Mathieu Desnoyers]

* Frank Ch. Eigler (fche@redhat.com) wrote:
> 
> Jon Masters <jcm@redhat.com> writes:
> 
> > I notice in module.c:
> >
> > #ifdef CONFIG_MARKERS
> > 	if (!mod->taints)
> > 		marker_update_probe_range(mod->markers,
> > 			mod->markers + mod->num_markers, NULL, NULL);
> > #endif
> >
> > Is this an attempt to not set a marker for proprietary modules? [...]
> 
> I can't seem to find any discussion about this aspect.  If this is the
> intent, it seems misguided to me.  There may instead be a relationship
> to TAINT_FORCED_{RMMOD,MODULE}.  Mathieu?
> 
> - FChE

On my part, its mostly a matter of not crashing the kernel when someone
tries to force modprobe of a proprietary module (where the checksums
doesn't match) on a kernel that supports the markers. Not doing so
causes the markers to try to find the marker-specific information in
struct module which doesn't exist and OOPSes.

Christoph's point of view is rather more drastic than mine : it's not
interesting for the kernel community to help proprietary modules writers,
so it's a good idea not to give them marker support. (I CC'ed him so he
can clarify his position).

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: CONFIG_MARKERS

[Jon Masters]

On Tue, 2008-01-22 at 22:10 -0500, Mathieu Desnoyers wrote:
> * Frank Ch. Eigler (fche@redhat.com) wrote:
> > 
> > Jon Masters <jcm@redhat.com> writes:
> > 
> > > I notice in module.c:
> > >
> > > #ifdef CONFIG_MARKERS
> > > 	if (!mod->taints)
> > > 		marker_update_probe_range(mod->markers,
> > > 			mod->markers + mod->num_markers, NULL, NULL);
> > > #endif
> > >
> > > Is this an attempt to not set a marker for proprietary modules? [...]
> > 
> > I can't seem to find any discussion about this aspect.  If this is the
> > intent, it seems misguided to me.  There may instead be a relationship
> > to TAINT_FORCED_{RMMOD,MODULE}.  Mathieu?
> > 
> > - FChE
> 
> On my part, its mostly a matter of not crashing the kernel when someone
> tries to force modprobe of a proprietary module (where the checksums
> doesn't match) on a kernel that supports the markers. Not doing so
> causes the markers to try to find the marker-specific information in
> struct module which doesn't exist and OOPSes.
> 
> Christoph's point of view is rather more drastic than mine : it's not
> interesting for the kernel community to help proprietary modules writers,
> so it's a good idea not to give them marker support. (I CC'ed him so he
> can clarify his position).

Right. I thought that was your collective opinion, and I happen to
personally agree with you, but my question was more that you should be
explicitly comparing to whether it's proprietary and not just whether
the taints field is set - there are other flags in there too.

Jon.

Re: CONFIG_MARKERS

[Frank Ch. Eigler]

Hi -

On Tue, Jan 22, 2008 at 11:17:40PM -0500, Jon Masters wrote:
> On Tue, 2008-01-22 at 22:10 -0500, Mathieu Desnoyers wrote:
> > [...]
> > > > Is this an attempt to not set a marker for proprietary modules? [...]
> > > 
> > > I can't seem to find any discussion about this aspect.  If this is the
> > > intent, it seems misguided to me.  There may instead be a relationship
> > > to TAINT_FORCED_{RMMOD,MODULE}.  Mathieu?

> > On my part, its mostly a matter of not crashing the kernel when someone
> > tries to force modprobe of a proprietary module (where the checksums
> > doesn't match) on a kernel that supports the markers. Not doing so
> > causes the markers to try to find the marker-specific information in
> > struct module which doesn't exist and OOPSes.

But you have the wrong target: it is not proprietary modules that have
this risk but those built out-of-tree without checksums.  Maybe
oopsing in this case is not so bad; or the check could just limit itself to
FORCED_MODULE.


> > Christoph's point of view is rather more drastic than mine : it's not
> > interesting for the kernel community to help proprietary modules writers,
> > so it's a good idea not to give them marker support. (I CC'ed him so he
> > can clarify his position).
> Right. I thought that was your collective opinion

Another way of looking at this though is that by allowing/encouraging
proprietary module writers to include markers, we and their users get
new diagnostic capabilities.  It constitutes a little bit of opening
up, which IMO we should reward rather than punish.


- FChE

Re: CONFIG_MARKERS

[Mathieu Desnoyers]

* Frank Ch. Eigler (fche@redhat.com) wrote:
> Hi -
> 
> On Tue, Jan 22, 2008 at 11:17:40PM -0500, Jon Masters wrote:
> > On Tue, 2008-01-22 at 22:10 -0500, Mathieu Desnoyers wrote:
> > > [...]
> > > > > Is this an attempt to not set a marker for proprietary modules? [...]
> > > > 
> > > > I can't seem to find any discussion about this aspect.  If this is the
> > > > intent, it seems misguided to me.  There may instead be a relationship
> > > > to TAINT_FORCED_{RMMOD,MODULE}.  Mathieu?
> 
> > > On my part, its mostly a matter of not crashing the kernel when someone
> > > tries to force modprobe of a proprietary module (where the checksums
> > > doesn't match) on a kernel that supports the markers. Not doing so
> > > causes the markers to try to find the marker-specific information in
> > > struct module which doesn't exist and OOPSes.
> 
> But you have the wrong target: it is not proprietary modules that have
> this risk but those built out-of-tree without checksums.  Maybe
> oopsing in this case is not so bad; or the check could just limit itself to
> FORCED_MODULE.
> 

I guess that for this one I could have a :

if (!mod->taints & TAINT_FORCED_MODULE)
 ...


> 
> > > Christoph's point of view is rather more drastic than mine : it's not
> > > interesting for the kernel community to help proprietary modules writers,
> > > so it's a good idea not to give them marker support. (I CC'ed him so he
> > > can clarify his position).
> > Right. I thought that was your collective opinion
> 
> Another way of looking at this though is that by allowing/encouraging
> proprietary module writers to include markers, we and their users get
> new diagnostic capabilities.  It constitutes a little bit of opening
> up, which IMO we should reward rather than punish.
> 
> 

This specific one is a kernel policy matter, and I personally don't
have a strong opinion about it. I agree that you raise a good counter
argument : it can be useful to proprietary modules users to be able to
extract tracing information from those modules to argue with their
vendors that their driver/hardware is broken (a tracer is _very_ useful
in that kind of situation). However, it is also useful to proprieraty
module writers who can benefit from the merged kernel/modules traces.
Do we want to give them this ability ? It would surely help writing
better proprieraty kernel modules. Do we want this, or rather prefer to
put more pressure on them so they open their code ?

I will let others fight in the mud on this one. :)

for this one, we could add, instead :

if (!mod->taints & (TAINT_FORCED_MODULE | TAINT_PROPRIETARY_MODULE))

Mathieu

> - FChE

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: CONFIG_MARKERS

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers@polymtl.ca) wrote:
> * Frank Ch. Eigler (fche@redhat.com) wrote:
> > Hi -
> > 
> > On Tue, Jan 22, 2008 at 11:17:40PM -0500, Jon Masters wrote:
> > > On Tue, 2008-01-22 at 22:10 -0500, Mathieu Desnoyers wrote:
> > > > [...]
> > > > > > Is this an attempt to not set a marker for proprietary modules? [...]
> > > > > 
> > > > > I can't seem to find any discussion about this aspect.  If this is the
> > > > > intent, it seems misguided to me.  There may instead be a relationship
> > > > > to TAINT_FORCED_{RMMOD,MODULE}.  Mathieu?
> > 
> > > > On my part, its mostly a matter of not crashing the kernel when someone
> > > > tries to force modprobe of a proprietary module (where the checksums
> > > > doesn't match) on a kernel that supports the markers. Not doing so
> > > > causes the markers to try to find the marker-specific information in
> > > > struct module which doesn't exist and OOPSes.
> > 
> > But you have the wrong target: it is not proprietary modules that have
> > this risk but those built out-of-tree without checksums.  Maybe
> > oopsing in this case is not so bad; or the check could just limit itself to
> > FORCED_MODULE.
> > 
> 
> I guess that for this one I could have a :
> 
> if (!mod->taints & TAINT_FORCED_MODULE)
>  ...
> 

as one could notice: missing parenthesis
if (!(mod->taints & TAINT_FORCED_MODULE))

> 
> > 
> > > > Christoph's point of view is rather more drastic than mine : it's not
> > > > interesting for the kernel community to help proprietary modules writers,
> > > > so it's a good idea not to give them marker support. (I CC'ed him so he
> > > > can clarify his position).
> > > Right. I thought that was your collective opinion
> > 
> > Another way of looking at this though is that by allowing/encouraging
> > proprietary module writers to include markers, we and their users get
> > new diagnostic capabilities.  It constitutes a little bit of opening
> > up, which IMO we should reward rather than punish.
> > 
> > 
> 
> This specific one is a kernel policy matter, and I personally don't
> have a strong opinion about it. I agree that you raise a good counter
> argument : it can be useful to proprietary modules users to be able to
> extract tracing information from those modules to argue with their
> vendors that their driver/hardware is broken (a tracer is _very_ useful
> in that kind of situation). However, it is also useful to proprieraty
> module writers who can benefit from the merged kernel/modules traces.
> Do we want to give them this ability ? It would surely help writing
> better proprieraty kernel modules. Do we want this, or rather prefer to
> put more pressure on them so they open their code ?
> 
> I will let others fight in the mud on this one. :)
> 
> for this one, we could add, instead :
> 
> if (!mod->taints & (TAINT_FORCED_MODULE | TAINT_PROPRIETARY_MODULE))
> 

here too
if (!(mod->taints & (TAINT_FORCED_MODULE | TAINT_PROPRIETARY_MODULE)))

Which remembers me to never write code before my first coffee in the
morning ;)

Mathieu

> Mathieu
> 
> > - FChE
> 
> -- 
> Mathieu Desnoyers
> Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
> OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: CONFIG_MARKERS

[Jon Masters]

On Wed, 2008-01-23 at 10:01 -0500, Mathieu Desnoyers wrote:

> if (!(mod->taints & (TAINT_FORCED_MODULE | TAINT_PROPRIETARY_MODULE)))
> 
> Which remembers me to never write code before my first coffee in the
> morning ;)

I switched to decaf and joined a gym...for someone who used to have 23
shots of espresso some days, it's a change.

But more importantly, are you going to take care of patching this up, or
shall I send something out? It's up to you.

Jon.

Re: CONFIG_MARKERS

[Mathieu Desnoyers]

* Jon Masters (jonathan@jonmasters.org) wrote:
> On Wed, 2008-01-23 at 10:01 -0500, Mathieu Desnoyers wrote:
> 
> > if (!(mod->taints & (TAINT_FORCED_MODULE | TAINT_PROPRIETARY_MODULE)))
> > 
> > Which remembers me to never write code before my first coffee in the
> > morning ;)
> 
> I switched to decaf and joined a gym...for someone who used to have 23
> shots of espresso some days, it's a change.
> 

I only have two a day, in the morning and exercise regularly. But still,
coding with only one eye opened is not recommended. :)

> But more importantly, are you going to take care of patching this up, or
> shall I send something out? It's up to you.
> 

I would prefer to wait until we have some clear statement on this issue
before I submit a patch (allowing markers in proprietary modules or
not).

As soon as we have an answer, I'll cook the one-liner.

Mathieu

> Jon.
> 
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: CONFIG_MARKERS

[Valdis.Kletnieks]

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

On Wed, 23 Jan 2008 09:48:12 EST, Mathieu Desnoyers said:

> This specific one is a kernel policy matter, and I personally don't
> have a strong opinion about it. I agree that you raise a good counter
> argument : it can be useful to proprietary modules users to be able to
> extract tracing information from those modules to argue with their
> vendors that their driver/hardware is broken (a tracer is _very_ useful
> in that kind of situation).

Amen, brother. Been there, done that, got the tshirt (not on Linux, but
other operating systems).

>                             However, it is also useful to proprieraty
> module writers who can benefit from the merged kernel/modules traces.
> Do we want to give them this ability ? 

The proprietary module writer has the *source* for the kernel and their module.
There's no way you can prevent the proprietary module writers from using this
feature as long as you allow other module writers to use it.

>                                           It would surely help writing
> better proprieraty kernel modules.

The biggest complaint against proprietary modules is that they make it
impossible for *us* to debug.  And you want to argue *against* a feature that
would allow them to develop better code that causes less crashes, and therefor
less people *asking* for us to debug it?

Remember - when a user tries a Linux box with a proprietary module, and the
experience sucks because the module sucks, they will walk away thinking
"Linux sucks", not "That module sucks".


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: CONFIG_MARKERS

[Jon Masters]

On Thu, 2008-01-24 at 00:25 -0500, Valdis.Kletnieks@vt.edu wrote:

> Remember - when a user tries a Linux box with a proprietary module, and the
> experience sucks because the module sucks, they will walk away thinking
> "Linux sucks", not "That module sucks".

Worse, if they're technically inclined, they'll think Linux sucks for
encoding philosophical policy into the kernel. Remember, a proprietary
driver is only "illegal" to distribute, it's not an infringement for
someone to write a non-GPL driver, or to have one on their computer.

Jon.

[PATCH] Linux Kernel Markers Support for Proprierary Modules

[Mathieu Desnoyers]

There seems to be good arguments for markers to support proprierary modules. So
I am throwing this one-liner in and let's see how people react. It only makes
sure that a module that has been "forced" to be loaded won't have its markers
used. It is important to leave this check to make sure the kernel does not crash
by expecting the markers part of the struct module by mistake in the case there
is an incorrect checksum.

It applies fine on 2.6.24-rc8-git3.

Discussion so far :
http://lkml.org/lkml/2008/1/22/226

Jon Masters <jcm@redhat.com> writes:
I notice in module.c:

#ifdef CONFIG_MARKERS
      if (!mod->taints)
              marker_update_probe_range(mod->markers,
                      mod->markers + mod->num_markers, NULL, NULL);
#endif

Is this an attempt to not set a marker for proprietary modules? [...]

* Frank Ch. Eigler (fche@redhat.com) wrote:
I can't seem to find any discussion about this aspect.  If this is the
intent, it seems misguided to me.  There may instead be a relationship
to TAINT_FORCED_{RMMOD,MODULE}.  Mathieu?

- FChE

On Tue, 2008-01-22 at 22:10 -0500, Mathieu Desnoyers wrote:
On my part, its mostly a matter of not crashing the kernel when someone
tries to force modprobe of a proprietary module (where the checksums
doesn't match) on a kernel that supports the markers. Not doing so
causes the markers to try to find the marker-specific information in
struct module which doesn't exist and OOPSes.

Christoph's point of view is rather more drastic than mine : it's not
interesting for the kernel community to help proprietary modules writers,
so it's a good idea not to give them marker support. (I CC'ed him so he
can clarify his position).

* Frank Ch. Eigler (fche@redhat.com) wrote:
[...]
Another way of looking at this though is that by allowing/encouraging
proprietary module writers to include markers, we and their users get
new diagnostic capabilities.  It constitutes a little bit of opening
up, which IMO we should reward rather than punish.


* Vladis Ketnieks (Valdis.Kletnieks@vt.edu) wrote:
On Wed, 23 Jan 2008 09:48:12 EST, Mathieu Desnoyers said:

> This specific one is a kernel policy matter, and I personally don't
> have a strong opinion about it. I agree that you raise a good counter
> argument : it can be useful to proprietary modules users to be able to
> extract tracing information from those modules to argue with their
> vendors that their driver/hardware is broken (a tracer is _very_ useful
> in that kind of situation).

Amen, brother. Been there, done that, got the tshirt (not on Linux, but
other operating systems).

>                             However, it is also useful to proprieraty
> module writers who can benefit from the merged kernel/modules traces.
> Do we want to give them this ability ?

The proprietary module writer has the *source* for the kernel and their module.
There's no way you can prevent the proprietary module writers from using this
feature as long as you allow other module writers to use it.

>                                           It would surely help writing
> better proprieraty kernel modules.

The biggest complaint against proprietary modules is that they make it
impossible for *us* to debug.  And you want to argue *against* a feature that
would allow them to develop better code that causes less crashes, and therefor
less people *asking* for us to debug it?

Remember - when a user tries a Linux box with a proprietary module, and the
experience sucks because the module sucks, they will walk away thinking
"Linux sucks", not "That module sucks".

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
CC: "Frank Ch. Eigler" <fche@redhat.com>
CC: Jon Masters <jcm@redhat.com>
CC: Rusty Russell <rusty@rustcorp.com.au>
CC: Christoph Hellwig <hch@infradead.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
---
 kernel/module.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-lttng/kernel/module.c
===================================================================
--- linux-2.6-lttng.orig/kernel/module.c	2008-01-24 07:29:10.000000000 -0500
+++ linux-2.6-lttng/kernel/module.c	2008-01-24 07:37:56.000000000 -0500
@@ -1992,7 +1992,7 @@ static struct module *load_module(void _
 	add_kallsyms(mod, sechdrs, symindex, strindex, secstrings);
 
 #ifdef CONFIG_MARKERS
-	if (!mod->taints)
+	if (!(mod->taints & TAINT_FORCED_MODULE))
 		marker_update_probe_range(mod->markers,
 			mod->markers + mod->num_markers, NULL, NULL);
 #endif
-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Valdis.Kletnieks]

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

On Thu, 24 Jan 2008 07:47:04 EST, Mathieu Desnoyers said:
> I am throwing this one-liner in and let's see how people react. It only makes
> sure that a module that has been "forced" to be loaded won't have its markers
> used. It is important to leave this check to make sure the kernel does not crash
> by expecting the markers part of the struct module by mistake in the case there
> is an incorrect checksum.

I can live with that - if anything, a force-loaded GPL module deserves to lose
even more than a non-GPL module built against the current kernel.  Quite
frankly, given that one of the reasons given for not liking closed modules is
"it's not maintainable", you'd *expect* that the infrastructure for allowing
a force-load of a module would have been thrown out entirely - is there anything
more unmaintainable than a module you *know* was built against different headers
and thus is using the wrong offsets for things?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Jon Masters]

On Thu, 2008-01-24 at 07:47 -0500, Mathieu Desnoyers wrote:
> There seems to be good arguments for markers to support proprierary modules. So
> I am throwing this one-liner in and let's see how people react. It only makes
> sure that a module that has been "forced" to be loaded won't have its markers
> used. It is important to leave this check to make sure the kernel does not crash
> by expecting the markers part of the struct module by mistake in the case there
> is an incorrect checksum.
> 
> It applies fine on 2.6.24-rc8-git3.

I think this should go in.

Signed-off-by: Jon Masters <jcm@jonmasters.org>

Jon.

P.S. wondering out loud to myself, I finally realized the reason we need
a leaf struct_module function in kernel/module.c. We don't necessarily
have anything else checking for changes to struct module on module load
without this, and we have an embedded struct module in each module that
we memory map as we load the module. I did wonder what was protecting us
from that (especially forced loads). But Rusty does think of everything.

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Rusty Russell]

On Thursday 24 January 2008 23:47:04 Mathieu Desnoyers wrote:
> There seems to be good arguments for markers to support proprierary
> modules.

My attitude to this is simple: who cares about proprietary modules?

The current test is wrong, it should be checking for forced module loads 
(which may not have marker information and may well crash the kernel).  Of 
course, all forced module loads can crash the kernel, but this is pretty 
certain and it's simply to avoid.

We should just flat-out refuse to load a module with a module section too 
small.  That will cover the majority of this case anyway (*and* the 
non-kallsysms case), and then we can remove this test altogether.

Cheers,
Rusty.

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Jan Engelhardt]

On Jan 24 2008 07:47, Mathieu Desnoyers wrote:
>On Tue, 2008-01-22 at 22:10 -0500, Mathieu Desnoyers wrote:
>On my part, its mostly a matter of not crashing the kernel when someone
>tries to force modprobe of a proprietary module (where the checksums
>doesn't match) on a kernel that supports the markers. Not doing so
>causes the markers to try to find the marker-specific information in
>struct module which doesn't exist and OOPSes.

>* Frank Ch. Eigler (fche@redhat.com) wrote:
>[...]
>Another way of looking at this though is that by allowing/encouraging
>proprietary module writers to include markers, we and their users get
>new diagnostic capabilities.  It constitutes a little bit of opening
>up, which IMO we should reward rather than punish.


Tackling this from a different angle:

I do not think there is a real reason to forceload a module, even
those with proprietary origin (vmware) or that are of
partially-closed nature (nvidia). vmware source is fully available,
so can be compiled with proper modinfo/vermagic/markers; nvidia uses
a build system trick to include an .o blob, but eventually its .ko
also ends up with a correct modinfo/vermagic.

Forceload is for people which like to trade an unstable system for
not having to install gcc and kernel-source.


>Remember - when a user tries a Linux box with a proprietary module, and the
>experience sucks because the module sucks, they will walk away thinking
>"Linux sucks", not "That module sucks".

So what is needed is an Oops with an explaining message
if (kernel_tainted) "blame that proprietary module first",
and make sure the user sees that oops even if in X.

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Valdis.Kletnieks]

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

On Fri, 25 Jan 2008 08:56:09 +0100, Jan Engelhardt said:
> So what is needed is an Oops with an explaining message
> if (kernel_tainted) "blame that proprietary module first",
> and make sure the user sees that oops even if in X.

The person who solves the "even if in X" problem will probably be nominated
for sainthood by the Linux community.  "It just hung with flashing LED's" is
just too common an event....


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Alan Cox]

On Fri, 25 Jan 2008 03:03:03 -0500
Valdis.Kletnieks@vt.edu wrote:

> On Fri, 25 Jan 2008 08:56:09 +0100, Jan Engelhardt said:
> > So what is needed is an Oops with an explaining message
> > if (kernel_tainted) "blame that proprietary module first",
> > and make sure the user sees that oops even if in X.
> 
> The person who solves the "even if in X" problem will probably be nominated
> for sainthood by the Linux community.  "It just hung with flashing LED's" is
> just too common an event....

I've been poking at it a bit but it will need X server help, or
eventually the kernel mode switch code.

Alan

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Jon Masters]

On Fri, 2008-01-25 at 08:56 +0100, Jan Engelhardt wrote:

> So what is needed is an Oops with an explaining message
> if (kernel_tainted) "blame that proprietary module first",
> and make sure the user sees that oops even if in X.

The former is actually trivially doable with the module->taints bits. We
could have the equivalent of a neon flashing "blame this module" sign.

I also agree, we should stop force loading. Incompatible struct module,
etc. are really bad things to have mapped into a running kernel.

Jon.

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Jan Engelhardt]

On Jan 25 2008 10:31, Jon Masters wrote:
>On Fri, 2008-01-25 at 08:56 +0100, Jan Engelhardt wrote:
>
>> So what is needed is an Oops with an explaining message
>> if (kernel_tainted) "blame that proprietary module first",
>> and make sure the user sees that oops even if in X.
>
>The former is actually trivially doable with the module->taints bits. We
>could have the equivalent of a neon flashing "blame this module" sign.
>
>I also agree, we should stop force loading. Incompatible struct module,
>etc. are really bad things to have mapped into a running kernel.

Forceloading should be reserved for developers who know
when a symversion change is safe (which is rare in itself,
but still).

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Rusty Russell]

On Saturday 26 January 2008 02:31:30 Jon Masters wrote:
> On Fri, 2008-01-25 at 08:56 +0100, Jan Engelhardt wrote:
> > So what is needed is an Oops with an explaining message
> > if (kernel_tainted) "blame that proprietary module first",
> > and make sure the user sees that oops even if in X.
>
> The former is actually trivially doable with the module->taints bits. We
> could have the equivalent of a neon flashing "blame this module" sign.
>
> I also agree, we should stop force loading. Incompatible struct module,
> etc. are really bad things to have mapped into a running kernel.

I think there are two things here:
1) Currently we allow modules with no kallsyms info to be loaded into a 
KALLSYMS kernel (and taint).  A new option is needed to control this: 
CONFIG_ACCEPT_NO_KALLSYMS under KERNEL_DEBUG which allows loading of 
such "stripped" modules (a-la modprobe --force).

2) Unconditionally reject modules with a wrong module section size.  Currently 
we have no such check, which means without KALLSYMS, anything goes.

Thoughts?
Rusty.

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Jon Masters]

On Sat, 2008-01-26 at 14:27 +1100, Rusty Russell wrote:

> 2) Unconditionally reject modules with a wrong module section size.  Currently 
> we have no such check, which means without KALLSYMS, anything goes.

I favor the latter, since it's safest.

Jon.

Re: [PATCH] Linux Kernel Markers Support for Proprierary Modules

[Jan Engelhardt]

On Jan 25 2008 23:21, Jon Masters wrote:
>On Sat, 2008-01-26 at 14:27 +1100, Rusty Russell wrote:
>
>> 2) Unconditionally reject modules with a wrong module section size.  Currently 
>> we have no such check, which means without KALLSYMS, anything goes.
>
>I favor the latter, since it's safest.

Is it possible to keep allowing the case where {section size is the
same and only the symversions are different}?

[PATCH] Linux Kernel Markers Support for Proprierary Modules

[Mathieu Desnoyers]

There seems to be good arguments for markers to support proprierary modules. So
I am throwing this one-liner in and let's see how people react. It only makes
sure that a module that has been "forced" to be loaded won't have its markers
used. It is important to leave this check to make sure the kernel does not crash
by expecting the markers part of the struct module by mistake in the case there
is an incorrect checksum.


Discussion so far :
http://lkml.org/lkml/2008/1/22/226

Jon Masters <jcm@redhat.com> writes:
I notice in module.c:

#ifdef CONFIG_MARKERS
      if (!mod->taints)
              marker_update_probe_range(mod->markers,
                      mod->markers + mod->num_markers, NULL, NULL);
#endif

Is this an attempt to not set a marker for proprietary modules? [...]

* Frank Ch. Eigler (fche@redhat.com) wrote:
I can't seem to find any discussion about this aspect.  If this is the
intent, it seems misguided to me.  There may instead be a relationship
to TAINT_FORCED_{RMMOD,MODULE}.  Mathieu?

- FChE

On Tue, 2008-01-22 at 22:10 -0500, Mathieu Desnoyers wrote:
On my part, its mostly a matter of not crashing the kernel when someone
tries to force modprobe of a proprietary module (where the checksums
doesn't match) on a kernel that supports the markers. Not doing so
causes the markers to try to find the marker-specific information in
struct module which doesn't exist and OOPSes.

Christoph's point of view is rather more drastic than mine : it's not
interesting for the kernel community to help proprietary modules writers,
so it's a good idea not to give them marker support. (I CC'ed him so he
can clarify his position).

* Frank Ch. Eigler (fche@redhat.com) wrote:
[...]
Another way of looking at this though is that by allowing/encouraging
proprietary module writers to include markers, we and their users get
new diagnostic capabilities.  It constitutes a little bit of opening
up, which IMO we should reward rather than punish.


* Vladis Ketnieks (Valdis.Kletnieks@vt.edu) wrote:
On Wed, 23 Jan 2008 09:48:12 EST, Mathieu Desnoyers said:

> This specific one is a kernel policy matter, and I personally don't
> have a strong opinion about it. I agree that you raise a good counter
> argument : it can be useful to proprietary modules users to be able to
> extract tracing information from those modules to argue with their
> vendors that their driver/hardware is broken (a tracer is _very_ useful
> in that kind of situation).

Amen, brother. Been there, done that, got the tshirt (not on Linux, but
other operating systems).

>                             However, it is also useful to proprieraty
> module writers who can benefit from the merged kernel/modules traces.
> Do we want to give them this ability ?

The proprietary module writer has the *source* for the kernel and their module.
There's no way you can prevent the proprietary module writers from using this
feature as long as you allow other module writers to use it.

>                                           It would surely help writing
> better proprieraty kernel modules.

The biggest complaint against proprietary modules is that they make it
impossible for *us* to debug.  And you want to argue *against* a feature that
would allow them to develop better code that causes less crashes, and therefor
less people *asking* for us to debug it?

Remember - when a user tries a Linux box with a proprietary module, and the
experience sucks because the module sucks, they will walk away thinking
"Linux sucks", not "That module sucks".

It applies on top of 2.6.24-git12.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
Acked-by: Jon Masters <jcm@jonmasters.org>
CC: "Frank Ch. Eigler" <fche@redhat.com>
CC: Jon Masters <jcm@redhat.com>
CC: Rusty Russell <rusty@rustcorp.com.au>
CC: Christoph Hellwig <hch@infradead.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
---
 kernel/module.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-lttng/kernel/module.c
===================================================================
--- linux-2.6-lttng.orig/kernel/module.c	2008-02-02 14:36:54.000000000 -0500
+++ linux-2.6-lttng/kernel/module.c	2008-02-02 14:36:56.000000000 -0500
@@ -2033,7 +2033,7 @@ static struct module *load_module(void _
 	add_kallsyms(mod, sechdrs, symindex, strindex, secstrings);
 
 #ifdef CONFIG_MARKERS
-	if (!mod->taints)
+	if (!(mod->taints & TAINT_FORCED_MODULE))
 		marker_update_probe_range(mod->markers,
 			mod->markers + mod->num_markers, NULL, NULL);
 #endif

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68