2.5.48: BUG() at kernel/module.c:1000

2.5.48: BUG() at kernel/module.c:1000

[Petr Vandrovec]

Hi Rusty,
  I'm trying to get VMware working, and unfortunately new insmod
is not able to load generated module. It died at line 1000 of 
kernel/module.c, because of used.core_size > mod->core_size:
     INIT=0/0  CORE=34252/34228

  Do you have any idea what's wrong with generated vmmon.o?
After I did "strip -R .note vmmon.o", module was insmod-dable.
Unfortunately lsmod now says 
"KBUILD_MODNAME         34220  0 [permanent]", although there is
no explanation in dmesg why it was marked [permanent] :-(

  If you are interested, generated vmmon.o is available at
http://vana.vc.cvut.cz/vmmon.o (if vana.vc.cvut.cz is alive)
(and yes, I fixed KBUILD_MODNAME module name here already...
but with vmmon in .modulename it does not die so spectacullary).

  And if we are talking about module names, I'm using
"insmod -o dummy0 dummy.o" & "insmod -o dummy1 dummy.o" to create
two dummy interfaces. What should I do now? Compile two dummy.o,
each with different module name?
						Thanks,
							Petr Vandrovec



/tmp/vmware-config0/vmmon.o:     file format elf32-i386

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00006520  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
  1 .rodata       00001b3f  00000000  00000000  00006560  2**5
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
  2 __ksymtab     00000040  00000000  00000000  000080a0  2**5
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
  3 .data         00000010  00000000  00000000  000080e0  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  4 .modulename   0000000f  00000000  00000000  000080f0  2**0
                  CONTENTS, ALLOC, LOAD, DATA
  5 .bss          000004ec  00000000  00000000  00008100  2**5
                  ALLOC
  6 .comment      00000150  00000000  00000000  00008100  2**0
                  CONTENTS, READONLY
  7 .note         0000008c  00000000  00000000  00008250  2**0
                  CONTENTS, READONLY

Re: 2.5.48: BUG() at kernel/module.c:1000

[mbm]

> Date: Mon, 18 Nov 2002 19:06:56 +0100
> From: Petr Vandrovec <vandrove@vc.cvut.cz>
> To: rusty@rustcorp.com.au
> Cc: linux-kernel@vger.kernel.org
> Subject: 2.5.48: BUG() at kernel/module.c:1000
> 
> Hi Rusty,
>   I'm trying to get VMware working, and unfortunately new insmod
> is not able to load generated module. It died at line 1000 of 
> kernel/module.c, because of used.core_size > mod->core_size:
>      INIT=0/0  CORE=34252/34228

Hi,

This appears to be due to the COMMON symbol "errno".

The code (get_sizes) that calculates the amount of space required
by the sections assumes that the first one is loaded at address
zero (or large alignment) when performing subsequent alignments.

Unfortunately, this is not the case when the actual load takes
place because the common area (length common_length) is allocated
first.  This needs to be rounded up to the strictest alignment of
any of the ALLOC sections before the copies start.  (Hence the
difference of (2**5 - 8) which is apparent in the CORE values above.)

cheers,

-Malcolm

Re: 2.5.48: BUG() at kernel/module.c:1000

[Rusty Russell]

In message <200211182239.gAIMdBL04074@mort.demon.co.uk> you write:
> The code (get_sizes) that calculates the amount of space required
> by the sections assumes that the first one is loaded at address
> zero (or large alignment) when performing subsequent alignments.

Please test this patch (Petr, contains fix for you too).

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

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.48/kernel/module.c working-2.5.48-fixes/kernel/module.c
--- linux-2.5.48/kernel/module.c	2002-11-19 09:58:52.000000000 +1100
+++ working-2.5.48-fixes/kernel/module.c	2002-11-19 10:33:48.000000000 +1100
@@ -788,9 +788,10 @@ static void simplify_symbols(Elf_Shdr *s
 /* Get the total allocation size of the init and non-init sections */
 static struct sizes get_sizes(const Elf_Ehdr *hdr,
 			      const Elf_Shdr *sechdrs,
-			      const char *secstrings)
+			      const char *secstrings,
+			      unsigned long common_length)
 {
-	struct sizes ret = { 0, 0 };
+	struct sizes ret = { 0, common_length };
 	unsigned i;
 
 	/* Everything marked ALLOC (this includes the exported
@@ -943,10 +944,9 @@ static struct module *load_module(void *
 	mod->live = 0;
 	module_unload_init(mod);
 
-	/* How much space will we need?  (Common area in core) */
-	sizes = get_sizes(hdr, sechdrs, secstrings);
-	common_length = read_commons(hdr, &sechdrs[symindex]);
-	sizes.core_size += common_length;
+	/* How much space will we need?  (Common area in first) */
+	sizes = get_sizes(hdr, sechdrs, secstrings,
+			  read_commons(hdr, &sechdrs[symindex]));
 
 	/* Set these up, and allow archs to manipulate them. */
 	mod->core_size = sizes.core_size;
@@ -973,7 +973,7 @@ static struct module *load_module(void *
 	mod->module_core = ptr;
 
 	ptr = module_alloc(mod->init_size);
-	if (!ptr) {
+	if (!ptr && mod->init_size) {
 		err = -ENOMEM;
 		goto free_core;
 	}

Re: 2.5.48: BUG() at kernel/module.c:1000

[Petr Vandrovec]

On Tue, Nov 19, 2002 at 10:50:42AM +1100, Rusty Russell wrote:
> In message <200211182239.gAIMdBL04074@mort.demon.co.uk> you write:
> > The code (get_sizes) that calculates the amount of space required
> > by the sections assumes that the first one is loaded at address
> > zero (or large alignment) when performing subsequent alignments.
> 
> Please test this patch (Petr, contains fix for you too).

Hi Rusty,
  I was getting oopses with your patch (due to uninitialized common_length).
With this one (against clean 2.5.48) it works fine (both parport
and vmmon can be insmodded/rmmoded (parport only until it is used by lp,
but that's another story)).

  I also modified copy_sections code to verify that we are not
overrunning allocated regions. So now you should get -ENOEXEC instead
of BUG() + corrupted kernel.
					Best regards,
						Petr Vandrovec
						vandrove@vc.cvut.cz


--- linux/kernel/module.c	2002-11-18 14:50:48.000000000 +0100
+++ linux/kernel/module.c	2002-11-19 12:49:37.000000000 +0100
@@ -607,14 +607,17 @@
 {
 	void *dest;
 	unsigned long *use;
+	unsigned long max;
 
 	/* Only copy to init section if there is one */
 	if (strstr(name, ".init") && mod->module_init) {
 		dest = mod->module_init;
 		use = &used->init_size;
+		max = mod->init_size;
 	} else {
 		dest = mod->module_core;
 		use = &used->core_size;
+		max = mod->core_size;
 	}
 
 	/* Align up */
@@ -622,6 +625,9 @@
 	dest += *use;
 	*use += sechdr->sh_size;
 
+	if (*use > max)
+		return ERR_PTR(-ENOEXEC);
+
 	/* May not actually be in the file (eg. bss). */
 	if (sechdr->sh_type != SHT_NOBITS)
 		memcpy(dest, base + sechdr->sh_offset, sechdr->sh_size);
@@ -788,9 +794,10 @@
 /* Get the total allocation size of the init and non-init sections */
 static struct sizes get_sizes(const Elf_Ehdr *hdr,
 			      const Elf_Shdr *sechdrs,
-			      const char *secstrings)
+			      const char *secstrings,
+			      unsigned long common_length)
 {
-	struct sizes ret = { 0, 0 };
+	struct sizes ret = { 0, common_length };
 	unsigned i;
 
 	/* Everything marked ALLOC (this includes the exported
@@ -943,10 +950,9 @@
 	mod->live = 0;
 	module_unload_init(mod);
 
-	/* How much space will we need?  (Common area in core) */
-	sizes = get_sizes(hdr, sechdrs, secstrings);
-	common_length = read_commons(hdr, &sechdrs[symindex]);
-	sizes.core_size += common_length;
+	/* How much space will we need?  (Common area in first) */
+	sizes = get_sizes(hdr, sechdrs, secstrings,
+			  common_length = read_commons(hdr, &sechdrs[symindex]));
 
 	/* Set these up, and allow archs to manipulate them. */
 	mod->core_size = sizes.core_size;
@@ -973,7 +979,7 @@
 	mod->module_core = ptr;
 
 	ptr = module_alloc(mod->init_size);
-	if (!ptr) {
+	if (!ptr && mod->init_size) {
 		err = -ENOMEM;
 		goto free_core;
 	}

Re: 2.5.48: BUG() at kernel/module.c:1000

[Rusty Russell]

In message <20021119115444.GA2011@vana> you write:
> On Tue, Nov 19, 2002 at 10:50:42AM +1100, Rusty Russell wrote:
> > In message <200211182239.gAIMdBL04074@mort.demon.co.uk> you write:
> > > The code (get_sizes) that calculates the amount of space required
> > > by the sections assumes that the first one is loaded at address
> > > zero (or large alignment) when performing subsequent alignments.
> > 
> > Please test this patch (Petr, contains fix for you too).
> 
> Hi Rusty,
>   I was getting oopses with your patch (due to uninitialized common_length).
> With this one (against clean 2.5.48) it works fine (both parport
> and vmmon can be insmodded/rmmoded (parport only until it is used by lp,
> but that's another story)).
> 
>   I also modified copy_sections code to verify that we are not
> overrunning allocated regions. So now you should get -ENOEXEC instead
> of BUG() + corrupted kernel.

Hmm, OK.  It *really* shouldn't happen unless there's a bug though.

Anyway, I made a tiny cleanup (I prefer not to do assignments inside
function parameters, it's icky).

Linus, please apply: fixes miscalculation of required module size due
to alignment issues, and also doesn't think that no init section is an
allocation failure.

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

Name: Module length calculation fix and module with no init fix
Author: Rusty Russell and Petr Vandrovec
Status: Tested on 2.5.48
Depends: Module/module.patch.gz

D: Fixes miscalculation of required module size due to alignment
D: issues of first section after common, and also doesn't think that
D: no init section is an allocation failure.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/kernel/module.c working-2.5-bk-spacefix/kernel/module.c
--- linux-2.5-bk/kernel/module.c	2002-11-20 05:58:00.000000000 +1100
+++ working-2.5-bk-spacefix/kernel/module.c	2002-11-20 17:47:45.000000000 +1100
@@ -592,14 +592,17 @@ static void *copy_section(const char *na
 {
 	void *dest;
 	unsigned long *use;
+	unsigned long max;
 
 	/* Only copy to init section if there is one */
 	if (strstr(name, ".init") && mod->module_init) {
 		dest = mod->module_init;
 		use = &used->init_size;
+		max = mod->init_size;
 	} else {
 		dest = mod->module_core;
 		use = &used->core_size;
+		max = mod->core_size;
 	}
 
 	/* Align up */
@@ -607,6 +610,9 @@ static void *copy_section(const char *na
 	dest += *use;
 	*use += sechdr->sh_size;
 
+	if (*use > max)
+		return ERR_PTR(-ENOEXEC);
+
 	/* May not actually be in the file (eg. bss). */
 	if (sechdr->sh_type != SHT_NOBITS)
 		memcpy(dest, base + sechdr->sh_offset, sechdr->sh_size);
@@ -773,9 +779,10 @@ static void simplify_symbols(Elf_Shdr *s
 /* Get the total allocation size of the init and non-init sections */
 static struct sizes get_sizes(const Elf_Ehdr *hdr,
 			      const Elf_Shdr *sechdrs,
-			      const char *secstrings)
+			      const char *secstrings,
+			      unsigned long common_length)
 {
-	struct sizes ret = { 0, 0 };
+	struct sizes ret = { 0, common_length };
 	unsigned i;
 
 	/* Everything marked ALLOC (this includes the exported
@@ -933,10 +940,9 @@ static struct module *load_module(void *
 	mod->live = 0;
 	module_unload_init(mod);
 
-	/* How much space will we need?  (Common area in core) */
-	sizes = get_sizes(hdr, sechdrs, secstrings);
+	/* How much space will we need?  (Common area in first) */
 	common_length = read_commons(hdr, &sechdrs[symindex]);
-	sizes.core_size += common_length;
+	sizes = get_sizes(hdr, sechdrs, secstrings, common_length);
 
 	/* Set these up, and allow archs to manipulate them. */
 	mod->core_size = sizes.core_size;
@@ -963,7 +969,7 @@ static struct module *load_module(void *
 	mod->module_core = ptr;
 
 	ptr = module_alloc(mod->init_size);
-	if (!ptr) {
+	if (!ptr && mod->init_size) {
 		err = -ENOMEM;
 		goto free_core;
 	}