[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 <[email protected]>
-------------------------------------------------------------------
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;
}
--
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 - http://www.grupopie.com
Pointy-Haired Boss: I don't see anything that could stand in our way.
Dilbert: Sanity? Reality? The laws of physics?
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
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.
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
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.
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]
"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
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
[email protected] 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