Re: changeset: Make forced module loading optional

Re: changeset: Make forced module loading optional

[Rusty Russell]

Linus's recent commit said:
> The kernel module loader used to be much too happy to allow loading of
> modules for the wrong kernel version by default. For example, if you
> had MODVERSIONS enabled, but tried to load a module with no version
> info, it would happily load it and taint the kernel - whether it was
> likely to actually work or not!
...
> Especially as it happened to me by mistake (ie regular unversioned Fedora
> modules getting loaded) causing lots of strange behavior.

Hi Linus,

   I'm trying to figure out how you did this.  So fedora builds unversioned 
modules, and version (and vermagic) matched your kernel?  And you somehow 
mixed them up?

   I don't think relying on modversions is the complete answer here.  Perhaps 
we should make modules_install blow away old modules?

Cheers,
Rusty.

Re: changeset: Make forced module loading optional

[Linus Torvalds]

On Mon, 5 May 2008, Rusty Russell wrote:
> 
>    I'm trying to figure out how you did this.  So fedora builds unversioned 
> modules, and version (and vermagic) matched your kernel?  And you somehow 
> mixed them up?

I don't use modules much, so many of my kernels tend to have modules off 
entirely. However, the Intel wireless drivers used to not work when 
built-in (fixed now, but I still had a legacy config), so my laptop had 
modules enabled, and MODVERSIONS set.

And I don't build initrd's etc crap, very much on purpose. I want to 
replace the kernel, nothing else, so my /etc/grub.conf file just replaces 
the distro kernel with my own, and keeps everything else untouched.

>    I don't think relying on modversions is the complete answer here.  Perhaps 
> we should make modules_install blow away old modules?

Wouldn't help one whit, and is against my rules anyway. See above. I want 
my own kernel, no other changes. That means that I run the distro initrd, 
which has its modules for bringing stuff up with distro kernels.

And quite frankly, when I finally figured out what was going on, I was 
like *WHAT THE HELL*. That kernel/module.c code was absolute and utter 
crap in accepting modules that neither matched the kernel version 
signature (because it had CONFIG_MODVERSIONS) *nor* the actual versioned 
symbols (because the distro modules had been built without 
CONFIG_MODVERSIONS).

So no, I'm not at all interested in blowing away old modules. I'm 
interested in having a module loader that isn't complete and utter crap 
and bypasses all the sanity checks that it has.

Which is what that changeset basically does.

People can still set CONFIG_MODULE_LOAD_FORCE, but quite frankly, I 
suspect that anybody who does that is just insane and/or works with 
proprietary and broken modules. So it's off by default, and hopefully no 
distro will ever set it.

			Linus

Re: changeset: Make forced module loading optional

[Rusty Russell]

On Monday 05 May 2008 15:05:51 Linus Torvalds wrote:
> On Mon, 5 May 2008, Rusty Russell wrote:
> >    I'm trying to figure out how you did this.  So fedora builds
> > unversioned modules, and version (and vermagic) matched your kernel?  And
> > you somehow mixed them up?
>
> And quite frankly, when I finally figured out what was going on, I was
> like *WHAT THE HELL*. That kernel/module.c code was absolute and utter
> crap in accepting modules that neither matched the kernel version
> signature (because it had CONFIG_MODVERSIONS) *nor* the actual versioned
> symbols (because the distro modules had been built without
> CONFIG_MODVERSIONS).

Erk, yes.  We ignore vermagic because we expect modversions, then ignore
missing modversions.  This is a logic bug; let's fix it.

BTW, for the peanut gallery: I don't recommend modversions: it's not reliable
in detecting all differences, nor being stable when there are no real
differences.

Untested:

module: don't ignore vermagic string if module doesn't have crcs

Linus found a logic bug: we ignore the version number in a module's
vermagic string if we have CONFIG_MODVERSIONS set, but modversions
also lets through a module with no versions at all (with tainting, but
still).

We should only ignore the start of the vermagic string if the module
actually *has* crcs to check.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 0cefb252efe8 kernel/module.c
--- a/kernel/module.c	Mon May 05 15:00:12 2008 +1000
+++ b/kernel/module.c	Mon May 05 15:25:17 2008 +1000
@@ -939,11 +939,14 @@ static inline int check_modstruct_versio
 	return check_version(sechdrs, versindex, "struct_module", mod, crc);
 }
 
-/* First part is kernel version, which we ignore. */
-static inline int same_magic(const char *amagic, const char *bmagic)
+/* First part is kernel version, which we ignore if module has crcs. */
+static inline int same_magic(const char *amagic, const char *bmagic,
+			     bool has_crcs)
 {
-	amagic += strcspn(amagic, " ");
-	bmagic += strcspn(bmagic, " ");
+	if (has_crcs) {
+		amagic += strcspn(amagic, " ");
+		bmagic += strcspn(bmagic, " ");
+	}
 	return strcmp(amagic, bmagic) == 0;
 }
 #else
@@ -963,7 +966,8 @@ static inline int check_modstruct_versio
 	return 1;
 }
 
-static inline int same_magic(const char *amagic, const char *bmagic)
+static inline int same_magic(const char *amagic, const char *bmagic,
+			     bool has_crcs)
 {
 	return strcmp(amagic, bmagic) == 0;
 }
@@ -1741,6 +1745,7 @@ static struct module *load_module(void _
 	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
 	struct exception_table_entry *extable;
 	mm_segment_t old_fs;
+	bool has_crcs = false;
 
 	DEBUGP("load_module: umod=%p, len=%lu, uargs=%p\n",
 	       umod, len, uargs);
@@ -1850,13 +1855,21 @@ static struct module *load_module(void _
 		goto free_hdr;
 	}
 
+#ifdef CONFIG_MODVERSIONS
+	if ((mod->num_syms == 0 || crcindex) &&
+	    (mod->num_gpl_syms == 0 || gplcrcindex) &&
+	    (mod->num_gpl_future_syms == 0 || gplfuturecrcindex) &&
+	    (mod->num_unused_syms == 0 || unusedcrcindex) &&
+	    (mod->num_unused_gpl_syms == 0 || unusedgplcrcindex))
+		has_crcs = true;
+#endif
 	modmagic = get_modinfo(sechdrs, infoindex, "vermagic");
 	/* This is allowed: modprobe --force will invalidate it. */
 	if (!modmagic) {
 		add_taint_module(mod, TAINT_FORCED_MODULE);
 		printk(KERN_WARNING "%s: no version magic, tainting kernel.\n",
 		       mod->name);
-	} else if (!same_magic(modmagic, vermagic)) {
+	} else if (!same_magic(modmagic, vermagic, has_crcs)) {
 		printk(KERN_ERR "%s: version magic '%s' should be '%s'\n",
 		       mod->name, modmagic, vermagic);
 		err = -ENOEXEC;
@@ -2001,11 +2014,8 @@ static struct module *load_module(void _
 			= (void *)sechdrs[unusedgplcrcindex].sh_addr;
 
 #ifdef CONFIG_MODVERSIONS
-	if ((mod->num_syms && !crcindex) ||
-	    (mod->num_gpl_syms && !gplcrcindex) ||
-	    (mod->num_gpl_future_syms && !gplfuturecrcindex) ||
-	    (mod->num_unused_syms && !unusedcrcindex) ||
-	    (mod->num_unused_gpl_syms && !unusedgplcrcindex)) {
+	/* If we get this far, it's time to warn about missing versions. */
+	if (!has_crcs) {
 		printk(KERN_WARNING "%s: No versions for exported symbols."
 		       " Tainting kernel.\n", mod->name);
 		add_taint_module(mod, TAINT_FORCED_MODULE);

Re: changeset: Make forced module loading optional

[Linus Torvalds]

On Mon, 5 May 2008, Rusty Russell wrote:
> 
> BTW, for the peanut gallery: I don't recommend modversions: it's not reliable
> in detecting all differences, nor being stable when there are no real
> differences.

Umm. modversions is in general a whole lot *more* reliable than just 
looking at the kernel version.

The kernel version is pretty good if you use CONFIG_LOCALVERSION_AUTO and 
have a git kernel tree, but if not, then modversions is much more likely 
to stop modules across big infrastructure changes during (say) the merge 
window.

So I agree that modversions is not "reliable", but I think that the 
alternative is often even *less* reliable, so I find the "don't recommend 
modversions" comment to be pretty debatable.

Since I personally try to avoid modules, and if I do use them I'd prefer 
the checking to be as strict as possible, I'd really not mind a "strict"  
mode that tests both MODVERSIONS _and_ the full kernel version string. 
Along with not allowing forced module loads, of course.

I also find it sad that apparently I'm one of the few ones that test with 
modules turned off. It's both more secure and simpler, but it does cause 
lots of noise at least during a Fedora boot, and it occasionally breaks 
the /etc/rc.d scripts because they assume that they have to load modules, 
and that it's an error if that fails. We had that happen with the iptables 
scripts not that long ago (and note how that was unrelated to initrd: this 
is past the point when things have switched to the normal root 
filesystem).

IOW, I wish distros did some testing with non-modular kernels too. Oh 
well. At least I can generally fix the problems, and make error reports, 
but I bet it means that most other kernel users simply turn on modules 
whether they need them or not.

			Linus

Re: changeset: Make forced module loading optional

[Rusty Russell]

On Tuesday 06 May 2008 03:07:03 Linus Torvalds wrote:
> On Mon, 5 May 2008, Rusty Russell wrote:
> > BTW, for the peanut gallery: I don't recommend modversions: it's not
> > reliable in detecting all differences, nor being stable when there are no
> > real differences.
...
> So I agree that modversions is not "reliable", but I think that the
> alternative is often even *less* reliable, so I find the "don't recommend
> modversions" comment to be pretty debatable.

Kids: do not shove random modules into your kernel.  Just because Linus does 
something doesn't make it a good idea.  modversions tries to be clever, but 
don't count on it; you want module signing for this (where did those patches 
go?)

BTW, I'm fascinated and a little nausiated that you ignore initrds.  We've 
moved half the kernel brains to userspace with udev, initrd and modules; it's 
really unfair that you're not sharing all that why-won't-my-machine-boot 
love.

Cheers,
Rusty.

Re: changeset: Make forced module loading optional

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Tue, 6 May 2008 04:42:12 +1000

> BTW, I'm fascinated and a little nausiated that you ignore initrds.  We've 
> moved half the kernel brains to userspace with udev, initrd and modules; it's 
> really unfair that you're not sharing all that why-won't-my-machine-boot 
> love.

It's pretty straightforward, if you ask me.  I avoid initrd's like
the plague too.

Building and compressing the initrd would increase my build+reboot
cycle time by at least 40%.

Add to that it's simply an enormous pain in the ass.

And for what?  I know what freakin' drivers to build statically into
my kernel.  I use raw partitions and don't use LVM, I know what
the heck my root filesystem device is named, etc.

The only thing I get "punished" for, where I am absolutely forced to
use an initrd, is when I use qlogic SCSI for the root partition, for
the firmware which we really should have kept in the tree :(

And this is not even a "licensing" issue, like it or not in-kernel
drivers are tied to specific firmware versions both from an API and
from a testing perspective.  Externalizing this is really a nightmare.

And I'm not avoiding udev by doing this.  udev works just fine if I
raw boot a kernel with all the static devices I need to get root
mounted.  All my networking etc. is modular and udev does the right
thing.

Re: changeset: Make forced module loading optional

[Jan Engelhardt]

On Monday 2008-05-05 07:05, Linus Torvalds wrote:
>On Mon, 5 May 2008, Rusty Russell wrote:
>
>And I don't build initrd's etc crap, very much on purpose. I want to
>replace the kernel, nothing else, so my /etc/grub.conf file just
>replaces the distro kernel with my own, and keeps everything else
>untouched. [...] I want my own kernel, no other changes. That means
>that I run the distro initrd, which has its modules for bringing
>stuff up with distro kernels.

That's not good procedure IMO. You always want to keep a known good
(=booting) kernel, and the distro-provided one might just be that.
So it's like:
	<hack hack hack>
	make modules_install
	cp arch/x86/boot/bzImage /boot/mykernel
	cp System.map /boot/System.map-2.6.26.1
	mkinitrd -i /boot/myinitrd -k /boot/mykernel # suse, fedora might slightly different
and you can constantly replace mykernel (resp. myinitrd via mkinitrd)
without having to touch grub.conf (that is, if you keep the name
'my*' over the development period), have modules, and the right ones
at that.

Re: changeset: Make forced module loading optional

[Linus Torvalds]

On Mon, 5 May 2008, Jan Engelhardt wrote:
> 
> That's not good procedure IMO. You always want to keep a known good
> (=booting) kernel, and the distro-provided one might just be that.

Yes, I've always had a known-good fallback. So I actually do duplicate the 
lines and leave unchanged versions in /etc/grub.conf for when things go 
wrong.

But my point is, I really want to change just the kernel. I don't want to 
care what the initrd does, and I don't want to build my own. It's all 
"user space" to me - and thus beneath my notice.

And the module loading bug was just that - a kernel bug. 

			Linus

Re: changeset: Make forced module loading optional

[Jeff Garzik]

Linus Torvalds wrote:
> But my point is, I really want to change just the kernel. I don't want to 
> care what the initrd does, and I don't want to build my own. It's all 
> "user space" to me - and thus beneath my notice.

True, but depending on the distro, the distro may want to load specific 
userland gadgets into the initrd based on your kernel build.

Most kernel hacker situations probably don't need this, but a modern 
distro these days _does_ want critical-to-boot stuff in initrd like LVM 
bootstrap programs, iSCSI discovery, Bluetooth userland management, etc.

	Jeff

Re: changeset: Make forced module loading optional

[Linus Torvalds]

On Mon, 5 May 2008, Jeff Garzik wrote:
> 
> Most kernel hacker situations probably don't need this, but a modern distro
> these days _does_ want critical-to-boot stuff in initrd like LVM bootstrap
> programs, iSCSI discovery, Bluetooth userland management, etc.

Right. Which is why I build the stuff into my kernel that is required, and 
then initrd can do whatever it damn well pleases.

			Linus

Re: changeset: Make forced module loading optional

[Linus Torvalds]

On Mon, 5 May 2008, Linus Torvalds wrote:
> 
> Right. Which is why I build the stuff into my kernel that is required, and 
> then initrd can do whatever it damn well pleases.

Btw, I realize that a distro *can* make an initrd that only works with 
their own kernel. I personally suspect I'll just switch distros if that 
happens - I'm already irritated enough with startup scripts etc that 
complain about "/proc/modules: No such file or directory" for stuff that 
is already compiled in because they want to check if it's loaded.

But quite frankly, the distro really shouldn't need to make their startup 
scripts (whether on initrd or just the regular /etc/rc.d/ ones) so 
fragile. And quite frankly, I don't understand why people seem to use a 
kernel *BUG* as an excuse to say that I shouldn't do what I have always 
done - replace just the kernel.

				Linus

Re: changeset: Make forced module loading optional

[Dave Jones]

On Mon, May 05, 2008 at 08:43:06AM +0200, Jan Engelhardt wrote:
 
 > So it's like:
 > 	<hack hack hack>
 > 	make modules_install
 > 	cp arch/x86/boot/bzImage /boot/mykernel
 > 	cp System.map /boot/System.map-2.6.26.1
 > 	mkinitrd -i /boot/myinitrd -k /boot/mykernel # suse, fedora might slightly different

For Fedora (and many other distros), you can replace the last three lines with 'make install'
It'll do everything. Construct the initrd. Install it & the vmlinuz, point grub.conf at it. etc.

	Dave

-- 
http://www.codemonkey.org.uk

Re: changeset: Make forced module loading optional

[Linus Torvalds]

On Mon, 5 May 2008, Dave Jones wrote:
> 
> For Fedora (and many other distros), you can replace the last three lines with 'make install'
> It'll do everything. Construct the initrd. Install it & the vmlinuz, point grub.conf at it. etc.

You guys aren't even listening, are you?

None of the above even *works* if you don't have modules enabled (which I 
mostly don't).

This bug reared its ugly head exactly because I normally don't do modules 
AT ALL, but had to have modules enabled on some of my machines due to 
Intel wireless legacy reasons.

Anyway, can we please agree to not beat a dead horse. If you cannot accept 
the fact that I want to just replace the kernel, then please don't even 
bother Cc'ing me in the discussion. Ok? Because if you cannot respect my 
choices, I sure as hell am not interested in hearing you blather about how 
I need to change how I use my kernel. You diss me, I diss you.

End of discussion.

		Linus

Re: changeset: Make forced module loading optional

[Jan Engelhardt]

On Monday 2008-05-05 17:32, Dave Jones wrote:
>On Mon, May 05, 2008 at 08:43:06AM +0200, Jan Engelhardt wrote:
> 
> > So it's like:
> > 	<hack hack hack>
> > 	make modules_install
> > 	cp arch/x86/boot/bzImage /boot/mykernel
> > 	cp System.map /boot/System.map-2.6.26.1
> > 	mkinitrd -i /boot/myinitrd -k /boot/mykernel # suse, fedora might slightly different
>
>For Fedora (and many other distros), you can replace the last three 
>lines with 'make install'

I don't trust that command. It might edit grub.conf in a way 
that somehow impacts the regular workflow. (Like, overwriting an old 
image, or creating oodles of entries in grub.conf.)
And say I had both lilo and grub installed (on the filesystem), with 
grub in the bootloader; I'd fear that make install might run lilo and 
trash grub in the MBR. No thanks, too much clouds. Also, it can't 
possibly know how to call mkinitrd, because every distro has its own 
stupid required flags (SUSE: requires none and tries to be smart, 
though it will be thankful if you pass it -i and -k, whereas on Fedora 
you need to specify the version, sigh.)

>It'll do everything. Construct the initrd. Install it & the vmlinuz, 
>point grub.conf at it. etc.

Re: changeset: Make forced module loading optional

[Alan Cox]

> trash grub in the MBR. No thanks, too much clouds. Also, it can't 
> possibly know how to call mkinitrd, because every distro has its own 
> stupid required flags (SUSE: requires none and tries to be smart, 

Of course it can - make install picks up distribution provided bits
beyond what is within the kernel scripting, and you can also use your own
scripts so make install does what you specifically want yourself.

Alan

Re: changeset: Make forced module loading optional

[Jan Engelhardt]

On Monday 2008-05-05 06:55, Rusty Russell wrote:
>Linus's recent commit said:
>
>   I don't think relying on modversions is the complete answer here.  Perhaps 
>we should make modules_install blow away old modules?

By default, modules_install already removes a great part of
/lib/modules/$(the version you just compiled) AFAIR before installing 
the new ones.