module version magic and arches with symbol prefixes

module version magic and arches with symbol prefixes

[Mike Frysinger]

the current check_modstruct_version() does this:
{
    const unsigned long *crc;

    if (!find_symbol("module_layout", NULL, &crc, true, false))
        BUG();
    return check_version(sechdrs, versindex, "module_layout", mod, crc);
}
the trouble here is that it looks for a literal "module_layout" symbol
and for ports that have symbol prefixes (a quick check shows Blackfin
& h8300), this aint going to work.

i tried to hack it in the Blackfin port by add this to the linker script:
module_layout = _module_layout
but that didnt seem to work.  maybe kallsysms couldnt find it or i
need to hack a different name ...

we could add a new function to asm-generic/sections.h:
#ifndef arch_symbol_name
#define arch_symbol_name(sym) sym
#endif
and in the case of Blackfin systems, we'd do:
#define arch_symbol_name(sym) "_" sym

no other consumer of find_symbol() has a problem because they're all
dynamic -- they scan the modules for required symbols and then scan
the kernel for those, so all the symbol prefixes line up.

that would fix the find_symbol() invocation, and check_version()
wouldnt need changing because in that case, it's look for the literal
symbol that scripts/mod/modpost.c is adding -- "module_layout" in this
case.

also, using BUG() here seems pretty damn harsh.  wouldnt it make more
sense to do something like:
    if (WARN_ON(!find_symbol("module_layout", NULL, &crc, true, false)))
        return 0;
this way the module is simply not loaded rather than killing the kernel
-mike

Re: module version magic and arches with symbol prefixes

[Rusty Russell]

On Fri, 19 Jun 2009 12:54:44 am Mike Frysinger wrote:
> the current check_modstruct_version() does this:
> {
>     const unsigned long *crc;
>
>     if (!find_symbol("module_layout", NULL, &crc, true, false))
>         BUG();
>     return check_version(sechdrs, versindex, "module_layout", mod, crc);
> }
> the trouble here is that it looks for a literal "module_layout" symbol
> and for ports that have symbol prefixes (a quick check shows Blackfin
> & h8300), this aint going to work.

MODULE_SYMBOL_PREFIX is the fix for this, ie:

	if (!find_symbol(MODULE_SYMBOL_PREFIX "module_layout), ...

> also, using BUG() here seems pretty damn harsh.  wouldnt it make more
> sense to do something like:
>     if (WARN_ON(!find_symbol("module_layout", NULL, &crc, true, false)))
>         return 0;
> this way the module is simply not loaded rather than killing the kernel

No, it means the kernel didn't build properly.  Better to fail spectacularly: 
ideally we'd do this find in an init routine and guarantee a boot crash.

Thanks,
Rusty.

[PATCH] module: use MODULE_SYMBOL_PREFIX with module_layout

[Mike Frysinger]

The check_modstruct_version() needs to look up the symbol "module_layout"
in the kernel, but it does so literally and not by a C identifier.  The
trouble is that it does not include a symbol prefix for those ports that
need it (like the Blackfin and H8300 port).  So make sure we tack on the
MODULE_SYMBOL_PREFIX define to the front of it.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 kernel/module.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 80036bc..7850bef 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1053,7 +1053,8 @@ static inline int check_modstruct_version(Elf_Shdr *sechdrs,
 {
 	const unsigned long *crc;
 
-	if (!find_symbol("module_layout", NULL, &crc, true, false))
+	if (!find_symbol(MODULE_SYMBOL_PREFIX "module_layout", NULL,
+	                 &crc, true, false))
 		BUG();
 	return check_version(sechdrs, versindex, "module_layout", mod, crc);
 }
-- 
1.6.3.1

Re: [PATCH] module: use MODULE_SYMBOL_PREFIX with module_layout

[Rusty Russell]

On Fri, 19 Jun 2009 05:19:22 pm Mike Frysinger wrote:
> The check_modstruct_version() needs to look up the symbol "module_layout"
> in the kernel, but it does so literally and not by a C identifier.  The
> trouble is that it does not include a symbol prefix for those ports that
> need it (like the Blackfin and H8300 port).  So make sure we tack on the
> MODULE_SYMBOL_PREFIX define to the front of it.

Thanks, applied.  I'm surprised this took so long to find.

Rusty.

Re: [PATCH] module: use MODULE_SYMBOL_PREFIX with module_layout

[Mike Frysinger]

On Fri, Jun 19, 2009 at 10:02, Rusty Russell wrote:
> On Fri, 19 Jun 2009 05:19:22 pm Mike Frysinger wrote:
>> The check_modstruct_version() needs to look up the symbol "module_layout"
>> in the kernel, but it does so literally and not by a C identifier.  The
>> trouble is that it does not include a symbol prefix for those ports that
>> need it (like the Blackfin and H8300 port).  So make sure we tack on the
>> MODULE_SYMBOL_PREFIX define to the front of it.
>
> Thanks, applied.  I'm surprised this took so long to find.

well, technically i first noticed on 2007-10-24, but the fix that went
in internally sucked and people forgot about it ;)
-mike

Re: module version magic and arches with symbol prefixes

[Robin Getz]

On Fri 19 Jun 2009 01:38, Rusty Russell pondered:
> On Fri, 19 Jun 2009 12:54:44 am Mike Frysinger wrote:
> > the current check_modstruct_version() does this:
> > {
> >     const unsigned long *crc;
> >
> >     if (!find_symbol("module_layout", NULL, &crc, true, false))
> >         BUG();
> >     return check_version(sechdrs, versindex, "module_layout", mod, crc);
> > }
> > the trouble here is that it looks for a literal "module_layout" symbol
> > and for ports that have symbol prefixes (a quick check shows Blackfin
> > & h8300), this aint going to work.
> 
> MODULE_SYMBOL_PREFIX is the fix for this, ie:
> 
> 	if (!find_symbol(MODULE_SYMBOL_PREFIX "module_layout), ...
> 
> > also, using BUG() here seems pretty damn harsh.  wouldnt it make more
> > sense to do something like:
> >     if (WARN_ON(!find_symbol("module_layout", NULL, &crc, true, false)))
> >         return 0;
> > this way the module is simply not loaded rather than killing the kernel
> 
> No, it means the kernel didn't build properly.

Or a module didn't build properly.

> Better to fail spectacularly:  
> ideally we'd do this find in an init routine and guarantee a boot crash.

But this isn't an init routine - this is something that runs when a module is 
loaded...

Anyone who can load a module can crash the kernel? I would expect the module 
not to load - but I agree with Mike - it seems a little harsh for someone who 
screwed up their module makefile....

-Robin

Re: module version magic and arches with symbol prefixes

[Mike Frysinger]

On Fri, Jun 19, 2009 at 08:54, Robin Getz wrote:
> On Fri 19 Jun 2009 01:38, Rusty Russell pondered:
>> On Fri, 19 Jun 2009 12:54:44 am Mike Frysinger wrote:
>> > the current check_modstruct_version() does this:
>> > {
>> >     const unsigned long *crc;
>> >
>> >     if (!find_symbol("module_layout", NULL, &crc, true, false))
>> >         BUG();
>> >     return check_version(sechdrs, versindex, "module_layout", mod, crc);
>> > }
>> > the trouble here is that it looks for a literal "module_layout" symbol
>> > and for ports that have symbol prefixes (a quick check shows Blackfin
>> > & h8300), this aint going to work.
>>
>> MODULE_SYMBOL_PREFIX is the fix for this, ie:
>>
>>       if (!find_symbol(MODULE_SYMBOL_PREFIX "module_layout), ...
>>
>> > also, using BUG() here seems pretty damn harsh.  wouldnt it make more
>> > sense to do something like:
>> >     if (WARN_ON(!find_symbol("module_layout", NULL, &crc, true, false)))
>> >         return 0;
>> > this way the module is simply not loaded rather than killing the kernel
>>
>> No, it means the kernel didn't build properly.
>
> Or a module didn't build properly.

nah, when i talked to you earlier i was wrong -- this find_symbol() is
looking up the symbol in the kernel, not in the module.  the lookup of
the symbol in the requested module actually works.
-mike

Re: module version magic and arches with symbol prefixes

[Robin Getz]

On Fri 19 Jun 2009 09:14, Mike Frysinger pondered:
> On Fri, Jun 19, 2009 at 08:54, Robin Getz wrote:
> > On Fri 19 Jun 2009 01:38, Rusty Russell pondered:
> >> No, it means the kernel didn't build properly.
> >
> > Or a module didn't build properly.
> 
> nah, when i talked to you earlier i was wrong -- this find_symbol() is
> looking up the symbol in the kernel, not in the module.  the lookup of
> the symbol in the requested module actually works.

OK - Yeah, I see it now in the kernel proper. (sorry for the noise)

Wouldn't it make sense to move this somewhere else then? and add a 
__initcall() for it? We should only need to check for it once - it would make 
things insignificantly faster, and smaller :)

Re: module version magic and arches with symbol prefixes

[Mike Frysinger]

On Fri, Jun 19, 2009 at 13:07, Robin Getz wrote:
> On Fri 19 Jun 2009 09:14, Mike Frysinger pondered:
>> On Fri, Jun 19, 2009 at 08:54, Robin Getz wrote:
>> > On Fri 19 Jun 2009 01:38, Rusty Russell pondered:
>> >> No, it means the kernel didn't build properly.
>> >
>> > Or a module didn't build properly.
>>
>> nah, when i talked to you earlier i was wrong -- this find_symbol() is
>> looking up the symbol in the kernel, not in the module.  the lookup of
>> the symbol in the requested module actually works.
>
> Wouldn't it make sense to move this somewhere else then? and add a
> __initcall() for it? We should only need to check for it once - it would make
> things insignificantly faster, and smaller :)

it would also have the advantage of the BUG() being noticed sooner ...
but up to Rusty i guess.
-mike

Re: module version magic and arches with symbol prefixes

[Rusty Russell]

On Sat, 20 Jun 2009 02:37:45 am Robin Getz wrote:
> On Fri 19 Jun 2009 09:14, Mike Frysinger pondered:
> > On Fri, Jun 19, 2009 at 08:54, Robin Getz wrote:
> > > On Fri 19 Jun 2009 01:38, Rusty Russell pondered:
> > >> No, it means the kernel didn't build properly.
> > >
> > > Or a module didn't build properly.
> >
> > nah, when i talked to you earlier i was wrong -- this find_symbol() is
> > looking up the symbol in the kernel, not in the module.  the lookup of
> > the symbol in the requested module actually works.
>
> OK - Yeah, I see it now in the kernel proper. (sorry for the noise)
>
> Wouldn't it make sense to move this somewhere else then? and add a
> __initcall() for it? We should only need to check for it once - it would
> make things insignificantly faster, and smaller :)

Patch welcome.

Rusty.

Re: module version magic and arches with symbol prefixes

[Robin Getz]

On Tue 23 Jun 2009 03:07, Rusty Russell pondered:
> On Sat, 20 Jun 2009 02:37:45 am Robin Getz wrote:
> > On Fri 19 Jun 2009 09:14, Mike Frysinger pondered:
> > > On Fri, Jun 19, 2009 at 08:54, Robin Getz wrote:
> > > > On Fri 19 Jun 2009 01:38, Rusty Russell pondered:
> > > >> No, it means the kernel didn't build properly.
> > > >
> > > > Or a module didn't build properly.
> > >
> > > nah, when i talked to you earlier i was wrong -- this find_symbol() is
> > > looking up the symbol in the kernel, not in the module.  the lookup of
> > > the symbol in the requested module actually works.
> >
> > OK - Yeah, I see it now in the kernel proper. (sorry for the noise)
> >
> > Wouldn't it make sense to move this somewhere else then? and add a
> > __initcall() for it? We should only need to check for it once - it would
> > make things insignificantly faster, and smaller :)
> 
> Patch welcome.

I looked at this:

> static inline int check_modstruct_version(Elf_Shdr *sechdrs,
>                                           unsigned int versindex,
>                                           struct module *mod)
> {
>         const unsigned long *crc;
>
>         if (!find_symbol(MODULE_SYMBOL_PREFIX "module_layout", NULL,
>                          &crc, true, false))
>                 BUG();
>         return check_version(sechdrs, versindex, "module_layout", mod, crc); }

and then decided that since you need to call find_symbol() to populate 
the crc, before it is used in check_version(), you might as well leave
alone (it wouldn't result in any smaller/faster code)...

So, I didn't send anything...