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
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.
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 <[email protected]>
---
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
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
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
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.
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 :)
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
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
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.
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...