[PATCH] Check compiler version, SMP and PREEMPT.

[PATCH] Check compiler version, SMP and PREEMPT.

[Rusty Russell]

Linus, please apply if you agree.

Tridge reported getting burned by gcc 3.2 compiled (Debian) XFree
modules not working on his gcc 2.95-compiled kernel.  Interestingly,
(as Tridge points out) modversions probably would not have caught the
change in spinlock size, since the ioctl takes a void*, not a
structure pointer...

Simple bitmask, allows extension later, and prevents this kind of
thing (maybe a warning is more appropriate: this refuses to load it).

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Module Sanity Check
Author: Rusty Russell
Status: Tested on 2.5.56

D: Stores a simple bitmask in the module structure, for SMP, PREEMPT
D: and compiler version (spinlocks change size on UP with gcc major,
D: at least).  Also printks on modules with common section (becoming
D: an FAQ for third-party modules).

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/include/linux/module.h working-2.5-bk-compilerversion/include/linux/module.h
--- linux-2.5-bk/include/linux/module.h	Mon Jan 13 11:17:32 2003
+++ working-2.5-bk-compilerversion/include/linux/module.h	Mon Jan 13 15:33:03 2003
@@ -32,6 +32,21 @@
 #define MODULE_SYMBOL_PREFIX ""
 #endif
 
+/* Simply sanity stamp to place in each module */
+#ifdef CONFIG_SMP
+#define MODULE_SANITY_SMP 1
+#else
+#define MODULE_SANITY_SMP 0
+#endif
+#ifdef CONFIG_PREEMPT
+#define MODULE_SANITY_PREEMPT 1
+#else
+#define MODULE_SANITY_PREEMPT 0
+#endif
+#define MODULE_SANITY						\
+	((MODULE_SANITY_SMP<<16) | (MODULE_SANITY_PREEMPT<<17) 	\
+	 | (__GNUC__<<8) | (__GNUC_MINOR__))
+
 #define MODULE_NAME_LEN (64 - sizeof(unsigned long))
 struct kernel_symbol
 {
@@ -168,6 +183,9 @@ struct module
 {
 	enum module_state state;
 
+	/* Simple compatibility bitmask (useful for non-modversions). */
+	int sanity;
+
 	/* Member of list of modules */
 	struct list_head list;
 
@@ -378,6 +396,7 @@ __attribute__((section(".gnu.linkonce.th
 #ifdef CONFIG_MODULE_UNLOAD
 	.exit = cleanup_module,
 #endif
+	.sanity = MODULE_SANITY,
 };
 #endif /* KBUILD_MODNAME */
 #endif /* MODULE */
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/kernel/module.c working-2.5-bk-compilerversion/kernel/module.c
--- linux-2.5-bk/kernel/module.c	Fri Jan 10 10:55:43 2003
+++ working-2.5-bk-compilerversion/kernel/module.c	Mon Jan 13 16:07:40 2003
@@ -846,6 +846,8 @@ static int simplify_symbols(Elf_Shdr *se
 			/* We compiled with -fno-common.  These are not
 			   supposed to happen.  */
 			DEBUGP("Common symbol: %s\n", strtab + sym[i].st_name);
+			printk("%s: probably not cimpiled with -fno-common\n",
+			       mod->name);
 			return -ENOEXEC;
 
 		case SHN_ABS:
@@ -1094,6 +1096,13 @@ static struct module *load_module(void *
 		goto free_hdr;
 	}
 	mod = (void *)sechdrs[modindex].sh_addr;
+
+	if (mod->sanity != MODULE_SANITY) {
+		printk(KERN_ERR "Module %s version %08x not %08x\n",
+		       mod->name, mod->sanity, MODULE_SANITY);
+		err = -ENOEXEC;
+		goto free_hdr;
+	}
 
 	/* Now copy in args */
 	err = strlen_user(uargs);

Re: [PATCH] Check compiler version, SMP and PREEMPT.

[Linus Torvalds]

On Mon, 13 Jan 2003, Rusty Russell wrote:
>
> Linus, please apply if you agree.

I don't like this, it doesn't make much sense to me to special-case this 
with some "module .sanity thing".

Instead, why don't you make _every_ object file just contain some magic
section (link-once), and then at module load time you compare the contents
of the section with the kernel magic section.

That magic section can then be arbitrary, with no strange bitmap 
limitations etc. 

		Linus

Re: [PATCH] Check compiler version, SMP and PREEMPT.

[Keith Owens]

On Mon, 13 Jan 2003 16:13:19 +1100, 
Rusty Russell <rusty@rustcorp.com.au> wrote:
>Linus, please apply if you agree.
>
>Tridge reported getting burned by gcc 3.2 compiled (Debian) XFree
>modules not working on his gcc 2.95-compiled kernel.  Interestingly,
>(as Tridge points out) modversions probably would not have caught the
>change in spinlock size, since the ioctl takes a void*, not a
>structure pointer...
>
>Simple bitmask, allows extension later, and prevents this kind of
>thing (maybe a warning is more appropriate: this refuses to load it).

Worse than that.  There is a long list of critical config options which
should :-

(a) Force a complete rebuild if any are changed and
(b) Refuse to load a module with different critical config options.

To make things more complicated, that list is arch dependent.

>From kbuild 2.5 (which handled this problem months ago)

define_string CONFIG_KBUILD_CRITICAL "CONFIG_SMP CONFIG_KBUILD_GCC_VERSION"
define_string CONFIG_KBUILD_CRITICAL_ARCH_X86 "CONFIG_M386 CONFIG_M486 \
       CONFIG_M586 CONFIG_M586TSC CONFIG_M586MMX CONFIG_M686 \
       CONFIG_MPENTIUMIII CONFIG_MPENTIUM4 \
       CONFIG_MK6 CONFIG_MK7 \
       CONFIG_MCRUSOE \
       CONFIG_MWINCHIPC6 CONFIG_MWINCHIP2 CONFIG_MWINCHIP3D \
       CONFIG_MCYRIXIII"

Re: [PATCH] Check compiler version, SMP and PREEMPT.

[Rusty Russell]

In message <Pine.LNX.4.44.0301122127130.1310-100000@home.transmeta.com> you wri
te:
> 
> On Mon, 13 Jan 2003, Rusty Russell wrote:
> >
> > Linus, please apply if you agree.
> 
> I don't like this, it doesn't make much sense to me to special-case this 
> with some "module .sanity thing".
> 
> Instead, why don't you make _every_ object file just contain some magic
> section (link-once), and then at module load time you compare the contents
> of the section with the kernel magic section.
> 
> That magic section can then be arbitrary, with no strange bitmap 
> limitations etc. 

Ok.  Not every object, but every object file which includes module.h.

I've only updated the x86 linker script, since the other archs'
compile will break as soon as they set CONFIG_MODULES=y (there are 40
linker scripts in the tree, and I don't want to patch them all again).

You now get:
ext2: version magic 'non-SMP,preempt,gcc-2.95' != kernel 'SMP,preempt,gcc-2.95'

Thanks!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Module Sanity Check
Author: Rusty Russell
Status: Tested on 2.5.56

D: Stores a section in each object, for SMP, PREEMPT and compiler
D: version (spinlocks change size on UP with gcc major, at least).
D: This was Linus' suggestion.  Also printks on modules with common
D: section (becoming an FAQ for third-party modules).

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/arch/i386/vmlinux.lds.S working-2.5-bk-sanityII/arch/i386/vmlinux.lds.S
--- linux-2.5-bk/arch/i386/vmlinux.lds.S	Mon Jan 13 16:56:21 2003
+++ working-2.5-bk-sanityII/arch/i386/vmlinux.lds.S	Mon Jan 13 17:36:39 2003
@@ -39,6 +39,9 @@ SECTIONS
   __kallsyms : { *(__kallsyms) }
   __stop___kallsyms = .;
 
+  __vermagic = . ;		/* Version magic to match against modules. */
+  .gnu.linkonce.vermagic : { *(.gnu.linkonce.vermagic) }
+
   /* writeable */
   .data : {			/* Data */
 	*(.data)
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/include/linux/module.h working-2.5-bk-sanityII/include/linux/module.h
--- linux-2.5-bk/include/linux/module.h	Mon Jan 13 16:56:29 2003
+++ working-2.5-bk-sanityII/include/linux/module.h	Mon Jan 13 17:27:41 2003
@@ -134,6 +134,25 @@ struct exception_table
 
 
 #ifdef CONFIG_MODULES
+/* Simply sanity version stamp to place in each object. */
+#ifdef CONFIG_SMP
+#define MODULE_VERMAGIC_SMP "SMP,"
+#else
+#define MODULE_VERMAGIC_SMP "non-SMP,"
+#endif
+#ifdef CONFIG_PREEMPT
+#define MODULE_VERMAGIC_PREEMPT "preempt,"
+#else
+#define MODULE_VERMAGIC_PREEMPT "non-preempt,"
+#endif
+#ifndef MODULE_ARCH_VERMAGIC
+#define MODULE_ARCH_VERMAGIC ""
+#endif
+
+char __vermagic[] __attribute__((section(".gnu.linkonce.vermagic"))) = 
+	MODULE_VERMAGIC_SMP MODULE_VERMAGIC_PREEMPT MODULE_ARCH_VERMAGIC 
+	"gcc-" __stringify(__GNUC__) "." __stringify(__GNUC_MINOR__);
+
 /* Get/put a kernel symbol (calls must be symmetric) */
 void *__symbol_get(const char *symbol);
 void *__symbol_get_gpl(const char *symbol);
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/kernel/module.c working-2.5-bk-sanityII/kernel/module.c
--- linux-2.5-bk/kernel/module.c	Mon Jan 13 16:56:30 2003
+++ working-2.5-bk-sanityII/kernel/module.c	Mon Jan 13 17:43:59 2003
@@ -846,6 +846,8 @@ static int simplify_symbols(Elf_Shdr *se
 			/* We compiled with -fno-common.  These are not
 			   supposed to happen.  */
 			DEBUGP("Common symbol: %s\n", strtab + sym[i].st_name);
+			printk("%s: probably not compiled with -fno-common\n",
+			       mod->name);
 			return -ENOEXEC;
 
 		case SHN_ABS:
@@ -976,6 +978,9 @@ static void set_license(struct module *m
 	}
 }
 
+/* Created bu the linker script */
+extern char __vermagic[];
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static struct module *load_module(void *umod,
@@ -986,7 +991,7 @@ static struct module *load_module(void *
 	Elf_Shdr *sechdrs;
 	char *secstrings, *args;
 	unsigned int i, symindex, exportindex, strindex, setupindex, exindex,
-		modindex, obsparmindex, licenseindex, gplindex;
+		modindex, obsparmindex, licenseindex, gplindex, vmagindex;
 	long arglen;
 	struct module *mod;
 	long err = 0;
@@ -1025,7 +1030,7 @@ static struct module *load_module(void *
 	exportindex = setupindex = obsparmindex = gplindex = licenseindex = 0;
 
 	/* And these should exist, but gcc whinges if we don't init them */
-	symindex = strindex = exindex = modindex = 0;
+	symindex = strindex = exindex = modindex = vmagindex = 0;
 
 	/* Find where important sections are */
 	for (i = 1; i < hdr->e_shnum; i++) {
@@ -1075,6 +1080,11 @@ static struct module *load_module(void *
 			/* EXPORT_SYMBOL_GPL() */
 			DEBUGP("GPL symbols found in section %u\n", i);
 			gplindex = i;
+		} else if (strcmp(secstrings+sechdrs[i].sh_name,
+				  ".gnu.linkonce.vermagic") == 0) {
+			/* Version magic. */
+			DEBUGP("Version magic found in section %u\n", i);
+			vmagindex = i;
 		}
 #ifdef CONFIG_KALLSYMS
 		/* symbol and string tables for decoding later. */
@@ -1094,6 +1104,16 @@ static struct module *load_module(void *
 		goto free_hdr;
 	}
 	mod = (void *)sechdrs[modindex].sh_addr;
+
+	if (!vmagindex
+	    || strcmp((char *)sechdrs[vmagindex].sh_addr, __vermagic) != 0) {
+		printk(KERN_WARNING "%s: version magic '%s' != kernel '%s'\n",
+		       mod->name,
+		       vmagindex ? (char*)sechdrs[vmagindex].sh_addr : "NONE",
+		       __vermagic);
+		err = -ENOEXEC;
+		goto free_hdr;
+	}
 
 	/* Now copy in args */
 	arglen = strlen_user(uargs);

Re: [PATCH] Check compiler version, SMP and PREEMPT.

[Arjan van de Ven]

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

On Mon, 2003-01-13 at 06:13, Rusty Russell wrote:
> Linus, please apply if you agree.
> 
> Tridge reported getting burned by gcc 3.2 compiled (Debian) XFree
> modules not working on his gcc 2.95-compiled kernel.

at least the other way around is detected by traditional modules
nowadays 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Check compiler version, SMP and PREEMPT.

[Daniel Jacobowitz]

On Mon, Jan 13, 2003 at 04:13:19PM +1100, Rusty Russell wrote:
> Linus, please apply if you agree.
> 
> Tridge reported getting burned by gcc 3.2 compiled (Debian) XFree
> modules not working on his gcc 2.95-compiled kernel.  Interestingly,
> (as Tridge points out) modversions probably would not have caught the
> change in spinlock size, since the ioctl takes a void*, not a
> structure pointer...

> D: and compiler version (spinlocks change size on UP with gcc major,
> D: at least).

Why does this happen?  It doesn't look like it should, but I only
skimmed the headers checking...

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH] Check compiler version, SMP and PREEMPT.

[Kai Germaschewski]

On Mon, 13 Jan 2003, Daniel Jacobowitz wrote:

> On Mon, Jan 13, 2003 at 04:13:19PM +1100, Rusty Russell wrote:
> > Linus, please apply if you agree.
> > 
> > Tridge reported getting burned by gcc 3.2 compiled (Debian) XFree
> > modules not working on his gcc 2.95-compiled kernel.  Interestingly,
> > (as Tridge points out) modversions probably would not have caught the
> > change in spinlock size, since the ioctl takes a void*, not a
> > structure pointer...
> 
> > D: and compiler version (spinlocks change size on UP with gcc major,
> > D: at least).
> 
> Why does this happen?  It doesn't look like it should, but I only
> skimmed the headers checking...

I suppose it's due to the

	#if (__GNUC__ > 2)
	[...]

in include/linux/spinlock.h, i.e. it's not at all gcc's fault.

--Kai

Re: [PATCH] Check compiler version, SMP and PREEMPT.

[Kai Germaschewski]

On Mon, 13 Jan 2003, Rusty Russell wrote:

> I've only updated the x86 linker script, since the other archs'
> compile will break as soon as they set CONFIG_MODULES=y (there are 40
> linker scripts in the tree, and I don't want to patch them all again).
> 
> You now get:
> ext2: version magic 'non-SMP,preempt,gcc-2.95' != kernel 'SMP,preempt,gcc-2.95'

I mostly agree with this, in particular since I've been planning to 
reimplement module version checking (not modversions, though that's on the 
list, too) for some time now.

My plan was to first of all use the normal version string ("2.5.55-preX") 
and add letters for the critical config options, like "S" for SMP, "P" for 
preempt and so on. However, following this discussion, I suppose using 
entire words like "SMP", "preempt" etc is clearer and nobody cares about 
saving 10 bytes in vmlinux.

I think it may, as kaos pointed out, also be necessary to allow for
architecture specific config options, like the processor type.

The implementation I have in mind would not generate the special section 
with that string inside a header but rather add it during the "ld -o 
module.ko" step (one of the reasons why I introduced that), in order to 
avoid unnecessary recompiles when e.g. the version changes from "-preN" to 
"-preN+1", which was a major concern people had with the old module 
version string.

Do you agree on doing it that way?

--Kai

Re: [PATCH] Check compiler version, SMP and PREEMPT.

[Rusty Russell]

In message <Pine.LNX.4.44.0301130938500.24477-100000@chaos.physics.uiowa.edu> y
ou write:
> On Mon, 13 Jan 2003, Rusty Russell wrote:
> 
> > I've only updated the x86 linker script, since the other archs'
> > compile will break as soon as they set CONFIG_MODULES=y (there are 40
> > linker scripts in the tree, and I don't want to patch them all again).
> > 
> > You now get:
> > ext2: version magic 'non-SMP,preempt,gcc-2.95' != kernel 'SMP,preempt,gcc-2
..95'
> 
> I think it may, as kaos pointed out, also be necessary to allow for
> architecture specific config options, like the processor type.

That's in there.  Look closer at the patch 8)

> The implementation I have in mind would not generate the special section 
> with that string inside a header but rather add it during the "ld -o 
> module.ko" step (one of the reasons why I introduced that), in order to 
> avoid unnecessary recompiles when e.g. the version changes from "-preN" to 
> "-preN+1", which was a major concern people had with the old module 
> version string.
> 
> Do you agree on doing it that way?

The time-to-recompile when version changes argument doesn't really
concern me (but of course, it does concern you 8).  My natural
aversion to changing the build system makes me tend away.

But get any solution past Linus and I'll be happy 8)

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.