[PATCH] fix for kallsyms module symbol resolution problem

[PATCH] fix for kallsyms module symbol resolution problem

[James Bottomley]

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

In lots of KALLSYMS symbol resolution in modules, I've noticed the
appearance of symbols with no names:

Jun 27 20:55:26 raven kernel:  [<10131440>] schedule_timeout+0x78/0xdc
Jun 27 20:55:26 raven kernel:  [<000f8240>] +0x4e0/0x598 [sunrpc]
Jun 27 20:55:26 raven kernel:  [<0014504c>] +0x150/0x43c [nfsd]
Jun 27 20:55:26 raven kernel:  [<10109c5c>] ret_from_kernel_thread+0x1c/0x24

The problem seems to be that get_ksymbol doesn't eliminate empty symbol
names when it does resolution.  The attached patch should fix this.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 518 bytes --]

===== kernel/module.c 1.87 vs edited =====
--- 1.87/kernel/module.c	Sat Jun 14 11:16:06 2003
+++ edited/kernel/module.c	Fri Jun 27 22:10:58 2003
@@ -1760,7 +1760,8 @@
 			continue;
 
 		if (mod->symtab[i].st_value <= addr
-		    && mod->symtab[i].st_value > mod->symtab[best].st_value)
+		    && mod->symtab[i].st_value > mod->symtab[best].st_value
+		    && *(mod->strtab + mod->symtab[i].st_name) != '\0')
 			best = i;
 		if (mod->symtab[i].st_value > addr
 		    && mod->symtab[i].st_value < nextval)

Re: [PATCH] fix for kallsyms module symbol resolution problem

[Rusty Russell]

In message <1056770789.1825.200.camel@mulgrave> you write:
> In lots of KALLSYMS symbol resolution in modules, I've noticed the
> appearance of symbols with no names:
> 
> Jun 27 20:55:26 raven kernel:  [<10131440>] schedule_timeout+0x78/0xdc
> Jun 27 20:55:26 raven kernel:  [<000f8240>] +0x4e0/0x598 [sunrpc]
> Jun 27 20:55:26 raven kernel:  [<0014504c>] +0x150/0x43c [nfsd]
> Jun 27 20:55:26 raven kernel:  [<10109c5c>] ret_from_kernel_thread+0x1c/0x24
> 
> The problem seems to be that get_ksymbol doesn't eliminate empty symbol
> names when it does resolution.  The attached patch should fix this.

Please test, because that's only one problem.

The other is that the module_text_address() returns true if the value
is within the module, *not* just if it's within a function.  So you
can get some noise there, too, on archs which don't do real
backtracing.

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

Re: [PATCH] fix for kallsyms module symbol resolution problem

[James Bottomley]

On Sun, 2003-06-29 at 21:06, Rusty Russell wrote:
> Please test, because that's only one problem.
> 
> The other is that the module_text_address() returns true if the value
> is within the module, *not* just if it's within a function.  So you
> can get some noise there, too, on archs which don't do real
> backtracing.

Well, the fix is pretty cast iron in that it will print out the closest
symbol with a non null name (which has got to be better than printing an
empty string).  The routine length may still be wrong since the next
closest symbol may still be null.

However, that does bring me to another issue we have on parisc (the
problem traceback was actually from x86): our tool chain, for reasons
best known to itself, inserts local symbol names into the symbol table
section.  On parisc, we get rid of them again by throwing them out of
the symbol table section in module_finalize (which we shouldn't do,
since the pointers are constant).

Perhaps there should be a per-arch hook for purging the symbol tables of
irrelevant symbols before we do kallsyms lookups in it?

James

Re: [PATCH] fix for kallsyms module symbol resolution problem

[Rusty Russell]

In message <1056942790.10904.324.camel@mulgrave> you write:
> On Sun, 2003-06-29 at 21:06, Rusty Russell wrote:
> > Please test, because that's only one problem.
> > 
> > The other is that the module_text_address() returns true if the value
> > is within the module, *not* just if it's within a function.  So you
> > can get some noise there, too, on archs which don't do real
> > backtracing.
> 
> Well, the fix is pretty cast iron in that it will print out the closest
> symbol with a non null name (which has got to be better than printing an
> empty string).  The routine length may still be wrong since the next
> closest symbol may still be null.

Yeah, but I was trying to get you to do more work.  And if the names
resulting are useless anyway, why apply the patch?

> Perhaps there should be a per-arch hook for purging the symbol tables of
> irrelevant symbols before we do kallsyms lookups in it?

I think you can do it easily in module_finalize... or if we were
ambitious we'd extract only the function symbols rather than keeping
the whole strtab and symtab.

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

Re: [PATCH] fix for kallsyms module symbol resolution problem

[James Bottomley]

On Mon, 2003-06-30 at 01:17, Rusty Russell wrote:
> Yeah, but I was trying to get you to do more work.  And if the names
> resulting are useless anyway, why apply the patch?

I noticed.

However, not printing empty names makes the trace a lot more useful. 
Just doing a symbolic trace on modules on x86, I see pretty much the
correct call trace now.

But, I'll investigate and see if I can find a way of generically telling
if a particular symbol is useful or not.

> > Perhaps there should be a per-arch hook for purging the symbol tables of
> > irrelevant symbols before we do kallsyms lookups in it?
> 
> I think you can do it easily in module_finalize... or if we were
> ambitious we'd extract only the function symbols rather than keeping
> the whole strtab and symtab.

Apart from the dubious writing to a const * pointer, yes I can (it's
what I'm doing now).  There is some annoyance in that module_finalize
isn't told where the string or symbol tables are, so I have to find them
again.

James

Re: [PATCH] fix for kallsyms module symbol resolution problem

[Rusty Russell]

In message <1056982249.2069.25.camel@mulgrave> you write:
> On Mon, 2003-06-30 at 01:17, Rusty Russell wrote:
> > Yeah, but I was trying to get you to do more work.  And if the names
> > resulting are useless anyway, why apply the patch?
> 
> I noticed.
> 
> However, not printing empty names makes the trace a lot more useful. 
> Just doing a symbolic trace on modules on x86, I see pretty much the
> correct call trace now.

Right.  *That* is what I wanted to know: whether those were really
part of the backtrace or just noise.  I'll forward to Linus.

> > I think you can do it easily in module_finalize... or if we were
> > ambitious we'd extract only the function symbols rather than keeping
> > the whole strtab and symtab.
> 
> Apart from the dubious writing to a const * pointer, yes I can (it's
> what I'm doing now).  There is some annoyance in that module_finalize
> isn't told where the string or symbol tables are, so I have to find them
> again.

My mistake: add_kallsyms should be above module_finalize, which means
you can just use the mod->symtab and mod->strtab members.

How's this?

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.73-bk7/kernel/module.c tmp/kernel/module.c
--- linux-2.5.73-bk7/kernel/module.c	2003-06-15 11:30:11.000000000 +1000
+++ tmp/kernel/module.c	2003-07-01 11:09:57.000000000 +1000
@@ -1349,7 +1349,15 @@ static void add_kallsyms(struct module *
 		mod->symtab[i].st_info
 			= elf_type(&mod->symtab[i], sechdrs, secstrings, mod);
 }
-#endif
+#else
+static inline void add_kallsyms(struct module *mod,
+				Elf_Shdr *sechdrs,
+				unsigned int symindex,
+				unsigned int strindex,
+				const char *secstrings)
+{
+}
+#endif /* CONFIG_KALLSYMS */
 
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
@@ -1603,14 +1611,12 @@ static struct module *load_module(void _
 	percpu_modcopy(mod->percpu, (void *)sechdrs[pcpuindex].sh_addr,
 		       sechdrs[pcpuindex].sh_size);
 
+	add_kallsyms(mod, sechdrs, symindex, strindex, secstrings);
+
 	err = module_finalize(hdr, sechdrs, mod);
 	if (err < 0)
 		goto cleanup;
 
-#ifdef CONFIG_KALLSYMS
-	add_kallsyms(mod, sechdrs, symindex, strindex, secstrings);
-#endif
-
 	mod->args = args;
 	if (obsparmindex) {
 		err = obsolete_params(mod->name, mod->args,
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] fix for kallsyms module symbol resolution problem

[James Bottomley]

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

On Mon, 2003-06-30 at 01:17, Rusty Russell wrote:
> Yeah, but I was trying to get you to do more work.  And if the names
> resulting are useless anyway, why apply the patch?

OK, how about the attached.  I think it solves the module_text_address()
problem too.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 3050 bytes --]

===== include/linux/module.h 1.65 vs edited =====
--- 1.65/include/linux/module.h	Fri Jun  6 00:54:38 2003
+++ edited/include/linux/module.h	Mon Jun 30 15:47:27 2003
@@ -217,6 +217,9 @@
 	/* Here are the sizes of the init and core sections */
 	unsigned long init_size, core_size;
 
+	/* The size of the code in each section.  */
+	unsigned long init_code_size, core_code_size;
+
 	/* Arch-specific module values */
 	struct mod_arch_specific arch;
 
===== kernel/module.c 1.86 vs edited =====
--- 1.86/kernel/module.c	Wed Jun 11 00:55:09 2003
+++ edited/kernel/module.c	Mon Jun 30 21:18:17 2003
@@ -1176,6 +1176,9 @@
 			    const char *secstrings)
 {
 	static unsigned long const masks[][2] = {
+		/* NOTE: code must be the first and only section
+		 * in this array; otherwise modify the code_size
+		 * finder in the two loops below */
 		{ SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
 		{ SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
 		{ SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
@@ -1198,6 +1201,8 @@
 				continue;
 			s->sh_entsize = get_offset(&mod->core_size, s);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
+			if(m == 0)
+				mod->core_code_size = mod->core_size;
 		}
 	}
 
@@ -1214,6 +1219,8 @@
 			s->sh_entsize = (get_offset(&mod->init_size, s)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
+			if(m == 0)
+				mod->init_code_size = mod->init_size;
 		}
 	}
 }
@@ -1726,6 +1733,7 @@
 	module_free(mod, mod->module_init);
 	mod->module_init = NULL;
 	mod->init_size = 0;
+	mod->init_code_size = 0;
 	up(&module_mutex);
 
 	return 0;
@@ -1747,9 +1755,11 @@
 
 	/* At worse, next value is at end of module */
 	if (within(addr, mod->module_init, mod->init_size))
-		nextval = (unsigned long)mod->module_init + mod->init_size;
+		nextval = (unsigned long)mod->module_init
+			+ mod->init_code_size;
 	else 
-		nextval = (unsigned long)mod->module_core + mod->core_size;
+		nextval = (unsigned long)mod->module_core
+			+ mod->core_code_size;
 
 	/* Scan for closest preceeding symbol, and next symbol. (ELF
            starts real symbols at 1). */
@@ -1758,10 +1768,12 @@
 			continue;
 
 		if (mod->symtab[i].st_value <= addr
-		    && mod->symtab[i].st_value > mod->symtab[best].st_value)
+		    && mod->symtab[i].st_value > mod->symtab[best].st_value
+		    && *(mod->strtab + mod->symtab[i].st_name) != '\0' )
 			best = i;
 		if (mod->symtab[i].st_value > addr
-		    && mod->symtab[i].st_value < nextval)
+		    && mod->symtab[i].st_value < nextval
+		    && *(mod->strtab + mod->symtab[i].st_name) != '\0')
 			nextval = mod->symtab[i].st_value;
 	}
 
@@ -1910,8 +1922,8 @@
 	struct module *mod;
 
 	list_for_each_entry(mod, &modules, list)
-		if (within(addr, mod->module_init, mod->init_size)
-		    || within(addr, mod->module_core, mod->core_size))
+		if (within(addr, mod->module_init, mod->init_code_size)
+		    || within(addr, mod->module_core, mod->core_code_size))
 			return mod;
 	return NULL;
 }

Re: [PATCH] fix for kallsyms module symbol resolution problem

[Rusty Russell]

In message <1057026277.2069.80.camel@mulgrave> you write:
> OK, how about the attached.  I think it solves the module_text_address()
> problem too.

Excellent: tested on bk8.  Linus, please apply (subsumes previous
patch: if you get a couple of rej's ignore them).

James: I changed the names to s/code/text/ since that's more expected
for toolchain-type people.  I frobbed the NOTE: comment a bit, and
moved the m == 0 tests outside the section loop.

Thanks!
Rusty.

================
Name: Identify Code Section Of Modules for kallsyms
Author: James Bottomley
Status: Tested on 2.5.73-bk8

D: Remember the size of the SHF_EXECINSTR sections, which are conveniently
D: at the start of the modules, and use that to more reliably implement
D: module_text_address().

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .5304-linux-2.5.73-bk8/include/linux/module.h .5304-linux-2.5.73-bk8.updated/include/linux/module.h
--- .5304-linux-2.5.73-bk8/include/linux/module.h	2003-06-15 11:30:08.000000000 +1000
+++ .5304-linux-2.5.73-bk8.updated/include/linux/module.h	2003-07-01 14:15:16.000000000 +1000
@@ -217,6 +217,9 @@ struct module
 	/* Here are the sizes of the init and core sections */
 	unsigned long init_size, core_size;
 
+	/* The size of the executable code in each section.  */
+	unsigned long init_text_size, core_text_size;
+
 	/* Arch-specific module values */
 	struct mod_arch_specific arch;
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .5304-linux-2.5.73-bk8/kernel/module.c .5304-linux-2.5.73-bk8.updated/kernel/module.c
--- .5304-linux-2.5.73-bk8/kernel/module.c	2003-06-15 11:30:11.000000000 +1000
+++ .5304-linux-2.5.73-bk8.updated/kernel/module.c	2003-07-01 14:37:18.000000000 +1000
@@ -1176,6 +1176,9 @@ static void layout_sections(struct modul
 			    const char *secstrings)
 {
 	static unsigned long const masks[][2] = {
+		/* NOTE: all executable code must be the first section
+		 * in this array; otherwise modify the text_size
+		 * finder in the two loops below */
 		{ SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
 		{ SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
 		{ SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
@@ -1199,6 +1202,8 @@ static void layout_sections(struct modul
 			s->sh_entsize = get_offset(&mod->core_size, s);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
+		if (m == 0)
+			mod->core_text_size = mod->core_size;
 	}
 
 	DEBUGP("Init section allocation order:\n");
@@ -1215,6 +1220,8 @@ static void layout_sections(struct modul
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
+		if (m == 0)
+			mod->init_text_size = mod->init_size;
 	}
 }
 
@@ -1726,6 +1733,7 @@ sys_init_module(void __user *umod,
 	module_free(mod, mod->module_init);
 	mod->module_init = NULL;
 	mod->init_size = 0;
+	mod->init_text_size = 0;
 	up(&module_mutex);
 
 	return 0;
@@ -1747,9 +1755,9 @@ static const char *get_ksymbol(struct mo
 
 	/* At worse, next value is at end of module */
 	if (within(addr, mod->module_init, mod->init_size))
-		nextval = (unsigned long)mod->module_init + mod->init_size;
+		nextval = (unsigned long)mod->module_init+mod->init_text_size;
 	else 
-		nextval = (unsigned long)mod->module_core + mod->core_size;
+		nextval = (unsigned long)mod->module_core+mod->core_text_size;
 
 	/* Scan for closest preceeding symbol, and next symbol. (ELF
            starts real symbols at 1). */
@@ -1757,11 +1765,15 @@ static const char *get_ksymbol(struct mo
 		if (mod->symtab[i].st_shndx == SHN_UNDEF)
 			continue;
 
+		/* We ignore unnamed symbols: they're uninformative
+		 * and inserted at a whim. */
 		if (mod->symtab[i].st_value <= addr
-		    && mod->symtab[i].st_value > mod->symtab[best].st_value)
+		    && mod->symtab[i].st_value > mod->symtab[best].st_value
+		    && *(mod->strtab + mod->symtab[i].st_name) != '\0' )
 			best = i;
 		if (mod->symtab[i].st_value > addr
-		    && mod->symtab[i].st_value < nextval)
+		    && mod->symtab[i].st_value < nextval
+		    && *(mod->strtab + mod->symtab[i].st_name) != '\0')
 			nextval = mod->symtab[i].st_value;
 	}
 
@@ -1910,8 +1924,8 @@ struct module *module_text_address(unsig
 	struct module *mod;
 
 	list_for_each_entry(mod, &modules, list)
-		if (within(addr, mod->module_init, mod->init_size)
-		    || within(addr, mod->module_core, mod->core_size))
+		if (within(addr, mod->module_init, mod->init_text_size)
+		    || within(addr, mod->module_core, mod->core_text_size))
 			return mod;
 	return NULL;
 }
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.