Re: [PATCH][RFC] 2.6 && module + -g && kernel w/o -g

Re: [PATCH][RFC] 2.6 && module + -g && kernel w/o -g

[Tom Rini]

On Wed, Jan 07, 2004 at 05:30:40PM -0700, Tom Rini wrote:

> Okay, this is what seems to fix the bug that hch found for me.  Does
> this seem right to everyone else?  I'm going to poke at it a bit more
> tomorrow, pending time.
> 
> ===== arch/ppc/kernel/module.c 1.10 vs edited =====
> --- 1.10/arch/ppc/kernel/module.c	Fri Sep 12 09:26:52 2003
> +++ edited//home/trini/work/kernel/testing/linux-2.6/arch/ppc/kernel/module.c	Wed Jan  7 17:07:30 2004
> @@ -87,6 +87,9 @@
>  		if ((strstr(secstrings + sechdrs[i].sh_name, ".init") != 0)
>  		    != is_init)
>  			continue;
> +		/* Skip over debug bits. */
> +		if (strstr(secstrings + sechdrs[i].sh_name, ".debug") != 0)
> +			continue;
>  
>  		if (sechdrs[i].sh_type == SHT_RELA) {
>  			DEBUGP("Found relocations in section %u\n", i);

Okay.  I've been looking at stock 2.6.1 noticed  that the fix for this
issue that Rusty proposed, and that ultimately made it into 2.6.1-rc3
(or so) is not correct.  The problem is that we do:

err = module_frob_arch_sections(hdr, sechdrs, secstrings, mod);
/* Which goes over every .debug section and can take _ages_ on something
 * like ipv6 */
... skip 100 lines ...
.... Checks Rusty / Linus added ...

The following patch fixes the problem for me on PPC32:

--- 1.96/kernel/module.c	Wed Jan  7 22:46:59 2004
+++ edited/kernel/module.c	Wed Jan 14 14:05:12 2004
@@ -1439,6 +1439,13 @@
 			strindex = sechdrs[i].sh_link;
 			strtab = (char *)hdr + sechdrs[strindex].sh_offset;
 		}
+
+		/* If we find any debug RELAs, frob these away now. */
+		if (sechdrs[i].sh_type == SHT_RELA &&
+				(strstr(secstrings+sechdrs[i].sh_name, ".debug")
+				 != 0))
+			sechdrs[i].sh_type = SHT_NULL;
+
 #ifndef CONFIG_MODULE_UNLOAD
 		/* Don't load .exit sections */
 		if (strncmp(secstrings+sechdrs[i].sh_name, ".exit", 5) == 0)

IMHO, this shouldn't be covered under a PPC32 test since at least PPC32,
PPC64 and Alpha have this issue, and I suspect that ia64, parisc, s390
and v850 have the problem as well (based on what their module_arch_frob
bits look to be doing).

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [PATCH][RFC] 2.6 && module + -g && kernel w/o -g

[David Mosberger]

>>>>> On Wed, 14 Jan 2004 14:09:37 -0700, Tom Rini <trini@kernel.crashing.org> said:

  Tom> The following patch fixes the problem for me on PPC32:

  Tom> --- 1.96/kernel/module.c	Wed Jan  7 22:46:59 2004
  Tom> +++ edited/kernel/module.c	Wed Jan 14 14:05:12 2004
  Tom> @@ -1439,6 +1439,13 @@
  Tom> strindex = sechdrs[i].sh_link;
  Tom> strtab = (char *)hdr + sechdrs[strindex].sh_offset;
  Tom> }
  Tom> +
  Tom> +		/* If we find any debug RELAs, frob these away now. */
  Tom> +		if (sechdrs[i].sh_type == SHT_RELA &&
  Tom> +				(strstr(secstrings+sechdrs[i].sh_name, ".debug")
  Tom> +				 != 0))
  Tom> +			sechdrs[i].sh_type = SHT_NULL;
  Tom> +
  Tom> #ifndef CONFIG_MODULE_UNLOAD
  Tom> /* Don't load .exit sections */
  Tom> if (strncmp(secstrings+sechdrs[i].sh_name, ".exit", 5) == 0)

  Tom> IMHO, this shouldn't be covered under a PPC32 test since at
  Tom> least PPC32, PPC64 and Alpha have this issue, and I suspect
  Tom> that ia64, parisc, s390 and v850 have the problem as well
  Tom> (based on what their module_arch_frob bits look to be doing).

As far as ia64 is concerned, adding a check for .debug should be OK,
but since the debug sections do not have any relocs anyhow, it
shouldn't make much of a difference one way or another (addresses in
the debug section a segment-relative).

	--david

Re: [PATCH][RFC] 2.6 && module + -g && kernel w/o -g

[Tom Rini]

On Wed, Jan 14, 2004 at 02:23:29PM -0800, David Mosberger wrote:
> >>>>> On Wed, 14 Jan 2004 14:09:37 -0700, Tom Rini <trini@kernel.crashing.org> said:
> 
>   Tom> The following patch fixes the problem for me on PPC32:
> 
>   Tom> --- 1.96/kernel/module.c	Wed Jan  7 22:46:59 2004
>   Tom> +++ edited/kernel/module.c	Wed Jan 14 14:05:12 2004
>   Tom> @@ -1439,6 +1439,13 @@
>   Tom> strindex = sechdrs[i].sh_link;
>   Tom> strtab = (char *)hdr + sechdrs[strindex].sh_offset;
>   Tom> }
>   Tom> +
>   Tom> +		/* If we find any debug RELAs, frob these away now. */
>   Tom> +		if (sechdrs[i].sh_type == SHT_RELA &&
>   Tom> +				(strstr(secstrings+sechdrs[i].sh_name, ".debug")
>   Tom> +				 != 0))
>   Tom> +			sechdrs[i].sh_type = SHT_NULL;
>   Tom> +
>   Tom> #ifndef CONFIG_MODULE_UNLOAD
>   Tom> /* Don't load .exit sections */
>   Tom> if (strncmp(secstrings+sechdrs[i].sh_name, ".exit", 5) == 0)
> 
>   Tom> IMHO, this shouldn't be covered under a PPC32 test since at
>   Tom> least PPC32, PPC64 and Alpha have this issue, and I suspect
>   Tom> that ia64, parisc, s390 and v850 have the problem as well
>   Tom> (based on what their module_arch_frob bits look to be doing).
> 
> As far as ia64 is concerned, adding a check for .debug should be OK,
> but since the debug sections do not have any relocs anyhow, it
> shouldn't make much of a difference one way or another (addresses in
> the debug section a segment-relative).

OK, I wasn't sure.  I just did a real quick skim of everyones module.c
to see if they did any for loops and checking of SHT_RELA.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [PATCH][RFC] 2.6 && module + -g && kernel w/o -g

[Rusty Russell]

In message <20040114210937.GA983@stop.crashing.org> you write:
> Okay.  I've been looking at stock 2.6.1 noticed  that the fix for this
> issue that Rusty proposed, and that ultimately made it into 2.6.1-rc3
> (or so) is not correct.  The problem is that we do:
> 
> err = module_frob_arch_sections(hdr, sechdrs, secstrings, mod);
> /* Which goes over every .debug section and can take _ages_ on something
>  * like ipv6 */

Right.  So the arch-specific module_frob_arch_sections() can be slow.
Logically, the fix should be in those module_frob_arch_sections(), not
in the generic code.

> +		/* If we find any debug RELAs, frob these away now. */
> +		if (sechdrs[i].sh_type == SHT_RELA &&
> +				(strstr(secstrings+sechdrs[i].sh_name, ".debug")
> +				 != 0))
> +			sechdrs[i].sh_type = SHT_NULL;
> +

Doesn't cover SHT_REL, and I really dislike name matches: they've bitten
us before.

Really, I prefer the arch-specific optimization.
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH][RFC] 2.6 && module + -g && kernel w/o -g

[Tom Rini]

On Thu, Jan 15, 2004 at 10:00:11AM +1100, Rusty Russell wrote:

> In message <20040114210937.GA983@stop.crashing.org> you write:
> > Okay.  I've been looking at stock 2.6.1 noticed  that the fix for this
> > issue that Rusty proposed, and that ultimately made it into 2.6.1-rc3
> > (or so) is not correct.  The problem is that we do:
> > 
> > err = module_frob_arch_sections(hdr, sechdrs, secstrings, mod);
> > /* Which goes over every .debug section and can take _ages_ on something
> >  * like ipv6 */
> 
> Right.  So the arch-specific module_frob_arch_sections() can be slow.
> Logically, the fix should be in those module_frob_arch_sections(), not
> in the generic code.

So it was right the first time, OK. :)

> > +		/* If we find any debug RELAs, frob these away now. */
> > +		if (sechdrs[i].sh_type == SHT_RELA &&
> > +				(strstr(secstrings+sechdrs[i].sh_name, ".debug")
> > +				 != 0))
> > +			sechdrs[i].sh_type = SHT_NULL;
> > +
> 
> Doesn't cover SHT_REL, and I really dislike name matches: they've bitten
> us before.
> 
> Really, I prefer the arch-specific optimization.

FWIW, this isn't an optimization, taking 12 minutes to load the ipv6
module is a bug. :)

Andrew, can you please apply the following patch?  Thanks.
--- 1.10/arch/ppc/kernel/module.c	Fri Sep 12 09:26:52 2003
+++ edited/arch/ppc/kernel/module.c	Thu Jan 15 08:35:40 2004
@@ -88,6 +88,10 @@
 		    != is_init)
 			continue;
 
+		/* We don't want to look at debug sections. */
+		if (strstr(secstrings + sechdrs[i].sh_name, ".debug") != 0)
+			continue;
+
 		if (sechdrs[i].sh_type == SHT_RELA) {
 			DEBUGP("Found relocations in section %u\n", i);
 			DEBUGP("Ptr: %p.  Number: %u\n",

-- 
Tom Rini
http://gate.crashing.org/~trini/