kallsyms_lookup_name should return the text addres

kallsyms_lookup_name should return the text addres

[Anil S Keshavamurthy]

On architectures like IA64, kallsyms_lookup_name(name) returns
the actual text address corresponding to the "name" and sometimes
returns address of the function descriptor, the behavior is
not consistent.

The bug is kallsyms_lookup_name() -> module_kallsyms_lookup_name(mod, name)
search the name in the given module and returns the address when
name is matched. This address very well could be the address of 'U' type
which is different address than 't' type.

Example:
Here is the output of cat /proc/kallsyms when we have test1.ko using the
my_test_reentrant_export_function.
-----------------------------------------------------------------
a00000020008c090 U my_test_reentrant_export_function    [test1]
a00000020008c0a0 r __ksymtab_my_test_reentrant_export_function  [mon_dummy]
a00000020008c0b0 r __kstrtab_my_test_reentrant_export_function  [mon_dummy]
a00000020008c0d8 r __kcrctab_my_test_reentrant_export_function  [mon_dummy]
00000000a356bab8 a __crc_my_test_reentrant_export_function      [mon_dummy]
a00000020008c000 T my_test_reentrant_export_function    [mon_dummy]
---------------------------------------------------------------

When we have test1.ko loaded, 
kallsyms_lookup_name(my_test_reentrant_export_function)
returns 0xa00000020008c090 which is a function descriptor address and 
when test1.ko is removed
kallsyms_lookup_name(my_test_reentrant_export_function)
returns 0xa00000020008c000 which is the actual text address

The patch following this mail fixes this issue.

-Anil Keshavamurthy

[patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Anil S Keshavamurthy]

[-- Attachment #1: kallsyms_lookup_name_fix.patch --]
[-- Type: text/plain, Size: 2615 bytes --]

[PATCH][BUG]kallsyms_lookup_name should return the text addres

On architectures like IA64, kallsyms_lookup_name(name) returns
the actual text address corresponding to the "name" and sometimes
returns address of the function descriptor, the behavior is
not consistent.

The bug is kallsyms_lookup_name() -> module_kallsyms_lookup_name(mod, name)
search the name in the given module and returns the address when
name is matched. This address very well could be the address of 'U' type
which is different address than 't' type.

Example:
Here is the output of cat /proc/kallsyms when we have test1.ko using the
my_test_reentrant_export_function.
-----------------------------------------------------------------
a00000020008c090 U my_test_reentrant_export_function    [test1]
a00000020008c0a0 r __ksymtab_my_test_reentrant_export_function  [mon_dummy]
a00000020008c0b0 r __kstrtab_my_test_reentrant_export_function  [mon_dummy]
a00000020008c0d8 r __kcrctab_my_test_reentrant_export_function  [mon_dummy]
00000000a356bab8 a __crc_my_test_reentrant_export_function      [mon_dummy]
a00000020008c000 T my_test_reentrant_export_function    [mon_dummy]
---------------------------------------------------------------

When we have test1.ko loaded, 
kallsyms_lookup_name(my_test_reentrant_export_function)
returns 0xa00000020008c090 which is a function descriptor address and 
when test1.ko is removed
kallsyms_lookup_name(my_test_reentrant_export_function)
returns 0xa00000020008c000 which is the actual text address

The current patch check for 't' type(text type) and always returns
text address. 

With this below fix, kallsyms_lookup_name(name) always 
returns consistent text address.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
-------------------------------------------------------------------

 kernel/module.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6.15-mm1/kernel/module.c
===================================================================
--- linux-2.6.15-mm1.orig/kernel/module.c
+++ linux-2.6.15-mm1/kernel/module.c
@@ -2085,13 +2085,14 @@ struct module *module_get_kallsym(unsign
 	up(&module_mutex);
 	return NULL;
 }
-
+/* Return the text address corresponding to this name */
 static unsigned long mod_find_symname(struct module *mod, const char *name)
 {
 	unsigned int i;
 
 	for (i = 0; i < mod->num_symtab; i++)
-		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0)
+		if ((strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0) &&
+			(mod->symtab[i].st_info == 't'))
 			return mod->symtab[i].st_value;
 	return 0;
 }

--

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Paulo Marques]

Anil S Keshavamurthy wrote:
> [...]
> The bug is kallsyms_lookup_name() -> module_kallsyms_lookup_name(mod, name)
> search the name in the given module and returns the address when
> name is matched. This address very well could be the address of 'U' type
> which is different address than 't' type.
> [...]

>  	for (i = 0; i < mod->num_symtab; i++)
> -		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0)
> +		if ((strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0) &&
> +			(mod->symtab[i].st_info == 't'))

This conflicts with a similar patch from Keith Owens earlier this week.

In his patch he did "mod->symtab[i].st_info != 'U'" instead of 
"mod->symtab[i].st_info == 't'".

I actually prefer Keith's version as it is the one which solves the bug 
by changing as least as possible the current behavior.

-- 
Paulo Marques - www.grupopie.com

Pointy-Haired Boss: I don't see anything that could stand in our way.
            Dilbert: Sanity? Reality? The laws of physics?

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Keshavamurthy Anil S]

On Tue, Jan 10, 2006 at 08:45:02PM +0000, Paulo Marques wrote:
> 
> This conflicts with a similar patch from Keith Owens earlier this week.
In fact I was the one who brought this issue to Keith and I missed seeing his
patch on the mailing list.

> I actually prefer Keith's version as it is the one which solves the bug 
> by changing as least as possible the current behavior.
That's fine, we can live with Keith's patch.
As long as the bug is solved, I am happy a man:-)

But my [patch 2/2] speeds up the lookup and that can go in, I think.
Please ack that patch if you think so.

-Anil

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Keith Owens]

Keshavamurthy Anil S (on Tue, 10 Jan 2006 13:07:37 -0800) wrote:
>On Tue, Jan 10, 2006 at 08:45:02PM +0000, Paulo Marques wrote:
>> 
>> This conflicts with a similar patch from Keith Owens earlier this week.
>In fact I was the one who brought this issue to Keith and I missed seeing his
>patch on the mailing list.
>
>> I actually prefer Keith's version as it is the one which solves the bug 
>> by changing as least as possible the current behavior.
>That's fine, we can live with Keith's patch.
>As long as the bug is solved, I am happy a man:-)
>
>But my [patch 2/2] speeds up the lookup and that can go in, I think.
>Please ack that patch if you think so.

Your second patch changes the behaviour of kallsyms lookup w.r.t
duplicate symbols.

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Keshavamurthy Anil S]

On Wed, Jan 11, 2006 at 10:11:26AM +1100, Keith Owens wrote:
> Keshavamurthy Anil S (on Tue, 10 Jan 2006 13:07:37 -0800) wrote:
> >On Tue, Jan 10, 2006 at 08:45:02PM +0000, Paulo Marques wrote:
> >But my [patch 2/2] speeds up the lookup and that can go in, I think.
> >Please ack that patch if you think so.
> 
> Your second patch changes the behaviour of kallsyms lookup w.r.t
> duplicate symbols.
With this send patch, kallsyms lookup first finds 
the real text address which is what we want. If you consider
this as the change in behaviour, what is the negetive effect of this
I am unable to get it.

In fact on arch which has the same address for 'U' and 't' type,
the search will first find the 't' type and ends the search soon, 
if we have my second patch.


regards,
-Anil

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Keith Owens]

Keshavamurthy Anil S (on Tue, 10 Jan 2006 15:29:05 -0800) wrote:
>On Wed, Jan 11, 2006 at 10:11:26AM +1100, Keith Owens wrote:
>> Keshavamurthy Anil S (on Tue, 10 Jan 2006 13:07:37 -0800) wrote:
>> >On Tue, Jan 10, 2006 at 08:45:02PM +0000, Paulo Marques wrote:
>> >But my [patch 2/2] speeds up the lookup and that can go in, I think.
>> >Please ack that patch if you think so.
>> 
>> Your second patch changes the behaviour of kallsyms lookup w.r.t
>> duplicate symbols.
>With this send patch, kallsyms lookup first finds 
>the real text address which is what we want. If you consider
>this as the change in behaviour, what is the negetive effect of this
>I am unable to get it.

Local symbols can be (and are) duplicated in the kernel code, and these
duplicate symbols can appear in modules.  Changing the list order of
loaded modules also changes which version of a duplicated symbol is
returned by the kallsyms code.  Not a big deal, but annoying enough to
say "don't change the module list order".

Changing the thread slightly, kallsyms_lookup_name() has never coped
with duplicate local symbols and it cannot do so without changing its
API, and all its callers.  For debugging purposes, it would be nicer if
the kernel did not have any duplicate symbols.  Perhaps some kernel
janitor would like to take that task on.

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Randy.Dunlap]

On Wed, 11 Jan 2006, Keith Owens wrote:

> Keshavamurthy Anil S (on Tue, 10 Jan 2006 15:29:05 -0800) wrote:
> >On Wed, Jan 11, 2006 at 10:11:26AM +1100, Keith Owens wrote:
> >> Keshavamurthy Anil S (on Tue, 10 Jan 2006 13:07:37 -0800) wrote:
> >> >On Tue, Jan 10, 2006 at 08:45:02PM +0000, Paulo Marques wrote:
> >> >But my [patch 2/2] speeds up the lookup and that can go in, I think.
> >> >Please ack that patch if you think so.
> >>
> >> Your second patch changes the behaviour of kallsyms lookup w.r.t
> >> duplicate symbols.
> >With this send patch, kallsyms lookup first finds
> >the real text address which is what we want. If you consider
> >this as the change in behaviour, what is the negetive effect of this
> >I am unable to get it.
>
> Local symbols can be (and are) duplicated in the kernel code, and these
> duplicate symbols can appear in modules.  Changing the list order of
> loaded modules also changes which version of a duplicated symbol is
> returned by the kallsyms code.  Not a big deal, but annoying enough to
> say "don't change the module list order".
>
> Changing the thread slightly, kallsyms_lookup_name() has never coped
> with duplicate local symbols and it cannot do so without changing its
> API, and all its callers.  For debugging purposes, it would be nicer if
> the kernel did not have any duplicate symbols.  Perhaps some kernel
> janitor would like to take that task on.

Jesper Juhl was doing some -Wshadow patches.  Would that detect
duplicate symbols?

-- 
~Randy  [sees nothing wrong with dup. local symbols, except for debugging]

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Keith Owens]

"Randy.Dunlap" (on Tue, 10 Jan 2006 16:07:55 -0800 (PST)) wrote:
>On Wed, 11 Jan 2006, Keith Owens wrote:
>> Changing the thread slightly, kallsyms_lookup_name() has never coped
>> with duplicate local symbols and it cannot do so without changing its
>> API, and all its callers.  For debugging purposes, it would be nicer if
>> the kernel did not have any duplicate symbols.  Perhaps some kernel
>> janitor would like to take that task on.
>
>Jesper Juhl was doing some -Wshadow patches.  Would that detect
>duplicate symbols?

No, the duplicate symbols are (a) static and (b) in separate source
files.  Run this against a System.map.

 awk '{print $NF}' System.map | egrep -v '^__ks|^__func' | sort | uniq -dc | LANG=C sort -k2

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Keshavamurthy Anil S]

On Wed, Jan 11, 2006 at 11:23:28AM +1100, Keith Owens wrote:
> "Randy.Dunlap" (on Tue, 10 Jan 2006 16:07:55 -0800 (PST)) wrote:
> >On Wed, 11 Jan 2006, Keith Owens wrote:
> >> Changing the thread slightly, kallsyms_lookup_name() has never coped
> >> with duplicate local symbols and it cannot do so without changing its
> >> API, and all its callers.  For debugging purposes, it would be nicer if
> >> the kernel did not have any duplicate symbols.  Perhaps some kernel
> >> janitor would like to take that task on.
> >
> >Jesper Juhl was doing some -Wshadow patches.  Would that detect
> >duplicate symbols?
> 
> No, the duplicate symbols are (a) static and (b) in separate source
> files.  Run this against a System.map.
> 
>  awk '{print $NF}' System.map | egrep -v '^__ks|^__func' | sort | uniq -dc | LANG=C sort -k2

Humm..This duplication of symbols in the kernel will be a 
problem for systemtap scripts, as we might end up putting probes
in the unwanted places :-(

I agree with you Keith, from the debugging purposes, it 
would make sense not to have any duplicate symbols.

Thanks,
Anil

Re: [patch 1/2] [BUG]kallsyms_lookup_name should return the text addres

[Frank Ch. Eigler]

anil.s.keshavamurthy@intel.com writes:

> [...]  Humm..This duplication of symbols in the kernel will be a
> problem for systemtap scripts, as we might end up putting probes in
> the unwanted places :-( [...]

Not at all.  Systemtap does not look in System.map.  It can qualify
function names with the compilation unit name to make unique the probe
target.  For that matter, it only uses /proc/kallsyms as a table to
drive the address-to-name mappings in debug output.

- FChE

[patch 2/2] Link new module to the tail of module list

[Anil S Keshavamurthy]

[-- Attachment #1: module_link_order.patch --]
[-- Type: text/plain, Size: 1350 bytes --]

[PATCH] Link new module to the tail of module list

When we are linking/adding a new module, it would be
better to insert the new module to the tail of the
module list. 

The reason is when kallsyms_lookup_name(name)
looks for the text address corresponding to the name
from the head of the module list, we always hit the
module exporting the text address first and then the
module using the text address later. This helps
kallsyms_lookup_name() search which indeed need
the text address.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
-------------------------------------------------------------------

 kernel/module.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletion(-)

Index: linux-2.6.15-mm1/kernel/module.c
===================================================================
--- linux-2.6.15-mm1.orig/kernel/module.c
+++ linux-2.6.15-mm1/kernel/module.c
@@ -1911,7 +1911,12 @@ static struct module *load_module(void _
 static int __link_module(void *_mod)
 {
 	struct module *mod = _mod;
-	list_add(&mod->list, &modules);
+	/* Insert the new modules at the tail of the list,
+	 * so kallsyms_lookup_name finds the module exporting
+	 * the text address of a function first and quickens
+	 * the search when searching based on function name
+	 */
+	list_add_tail(&mod->list, &modules);
 	return 0;
 }
 

--