From: Robin Getz <[email protected]>
Today, when module names are looked up, we do not qualify them (check to see
if the init section is still active or not). This can lead to problems when
kernel modules get loaded into the same address that the kernel init section
(or other module's init section was at). We sometimes return the old / no
lomnger there
This leads to bogus OOPS messages, and developers wasting their time looking
for problems (in the kernel init section) where there are none (since it was
a module).
This patch qualifies the addresses, to make sure the addresses are still valid
before label/offset.
Signed-off-by: Robin Getz <[email protected]>
---
linux-2.6.x/kernel/kallsyms.c | 3 ++-
linux-2.6.x/kernel/module.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6.x/kernel/kallsyms.c
===================================================================
--- linux-2.6.x/kernel/kallsyms.c (revision 4212)
+++ linux-2.6.x/kernel/kallsyms.c (working copy)
@@ -42,7 +42,8 @@
static inline int is_kernel_inittext(unsigned long addr)
{
- if (addr >= (unsigned long)_sinittext
+ if (system_state == SYSTEM_BOOTING
+ && addr >= (unsigned long)_sinittext
&& addr <= (unsigned long)_einittext)
return 1;
return 0;
Index: linux-2.6.x/kernel/module.c
===================================================================
--- linux-2.6.x/kernel/module.c (revision 4212)
+++ linux-2.6.x/kernel/module.c (working copy)
@@ -2121,7 +2121,8 @@
struct module *mod;
list_for_each_entry(mod, &modules, list) {
- if (within(addr, mod->module_init, mod->init_size)
+ if ((within(addr, mod->module_init, mod->init_size)
+ && mod->state == MODULE_STATE_COMING)
|| within(addr, mod->module_core, mod->core_size)) {
if (modname)
*modname = mod->name;
On Tue, 4 Mar 2008 18:47:15 -0500
Robin Getz <[email protected]> wrote:
> From: Robin Getz <[email protected]>
>
> Today, when module names are looked up, we do not qualify them (check to see
> if the init section is still active or not). This can lead to problems when
> kernel modules get loaded into the same address that the kernel init section
> (or other module's init section was at). We sometimes return the old / no
> lomnger there
>
> This leads to bogus OOPS messages, and developers wasting their time looking
> for problems (in the kernel init section) where there are none (since it was
> a module).
>
> This patch qualifies the addresses, to make sure the addresses are still valid
> before label/offset.
>
> Signed-off-by: Robin Getz <[email protected]>
>
> ---
>
> linux-2.6.x/kernel/kallsyms.c | 3 ++-
> linux-2.6.x/kernel/module.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> Index: linux-2.6.x/kernel/kallsyms.c
> ===================================================================
> --- linux-2.6.x/kernel/kallsyms.c (revision 4212)
> +++ linux-2.6.x/kernel/kallsyms.c (working copy)
> @@ -42,7 +42,8 @@
>
> static inline int is_kernel_inittext(unsigned long addr)
> {
> - if (addr >= (unsigned long)_sinittext
> + if (system_state == SYSTEM_BOOTING
> + && addr >= (unsigned long)_sinittext
> && addr <= (unsigned long)_einittext)
> return 1;
> return 0;
> Index: linux-2.6.x/kernel/module.c
> ===================================================================
> --- linux-2.6.x/kernel/module.c (revision 4212)
> +++ linux-2.6.x/kernel/module.c (working copy)
> @@ -2121,7 +2121,8 @@
> struct module *mod;
>
> list_for_each_entry(mod, &modules, list) {
> - if (within(addr, mod->module_init, mod->init_size)
> + if ((within(addr, mod->module_init, mod->init_size)
> + && mod->state == MODULE_STATE_COMING)
> || within(addr, mod->module_core, mod->core_size)) {
> if (modname)
> *modname = mod->name;
Both of the above additions could do with a comment explaining what's going
on.
The first one perhaps should use the more specific initmem_now_dynamic
which is added by
gregkh-driver-warn-when-statically-allocated-kobjects-are-used.patch from
Greg's driver tree. If the intent is to merge that - if not perhaps it can
be split up.
On Wednesday 05 March 2008 10:47:15 Robin Getz wrote:
> From: Robin Getz <[email protected]>
>
> Today, when module names are looked up, we do not qualify them (check to
> see if the init section is still active or not). This can lead to problems
> when kernel modules get loaded into the same address that the kernel init
> section (or other module's init section was at). We sometimes return the
> old / no lomnger there
>
> This leads to bogus OOPS messages, and developers wasting their time
> looking for problems (in the kernel init section) where there are none
> (since it was a module).
Hi Robin,
This is a great explanation, with only one problem: it isn't true.
mod->init_size is set to zero after init.
Kernel submitters learn not to express doubts about their patches, lest
they be dropped. But it makes the job of maintainers even harder, since we
don't know what's tested and what's an educated guess.
As to the actual patch, your kallsyms.c patch matches
a2da4052f1df6bc77749f84496fe731ab8b458f7's change to extable.c: please
resubmit with just that one. For bonus points, look at combining the extable
and kallsyms logic so we don't diverge in future...
Thanks!
Rusty.
On Tue 4 Mar 2008 20:43, Rusty Russell pondered:
> On Wednesday 05 March 2008 10:47:15 Robin Getz wrote:
> > From: Robin Getz <[email protected]>
> >
> > Today, when module names are looked up, we do not qualify them (check to
> > see if the init section is still active or not). This can lead to problems
> > when kernel modules get loaded into the same address that the kernel init
> > section (or other module's init section was at). We sometimes return the
> > old / no lomnger there
> >
> > This leads to bogus OOPS messages, and developers wasting their time
> > looking for problems (in the kernel init section) where there are none
> > (since it was a module).
>
> Hi Robin,
>
> This is a great explanation, with only one problem: it isn't true.
I can replicate this on 2.6.24.3 (at least on noMMU) in the kernel init
section.
> mod->init_size is set to zero after init.
So, I have seen the problem on the init section, and suspected it could happen
on module init, but it looks like I missed that portion of the code - sorry.
> As to the actual patch, your kallsyms.c patch matches
> a2da4052f1df6bc77749f84496fe731ab8b458f7's change to extable.c: please
> resubmit with just that one.
Will do.
> For bonus points, look at combining the
> extable and kallsyms logic so we don't diverge in future...
Let me see what I can do.
On Tue, Mar 04, 2008 at 04:05:01PM -0800, Andrew Morton wrote:
> On Tue, 4 Mar 2008 18:47:15 -0500
> Robin Getz <[email protected]> wrote:
>
> > From: Robin Getz <[email protected]>
> >
> > Today, when module names are looked up, we do not qualify them (check to see
> > if the init section is still active or not). This can lead to problems when
> > kernel modules get loaded into the same address that the kernel init section
> > (or other module's init section was at). We sometimes return the old / no
> > lomnger there
> >
> > This leads to bogus OOPS messages, and developers wasting their time looking
> > for problems (in the kernel init section) where there are none (since it was
> > a module).
> >
> > This patch qualifies the addresses, to make sure the addresses are still valid
> > before label/offset.
> >
> > Signed-off-by: Robin Getz <[email protected]>
> >
> > ---
> >
> > linux-2.6.x/kernel/kallsyms.c | 3 ++-
> > linux-2.6.x/kernel/module.c | 3 ++-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > Index: linux-2.6.x/kernel/kallsyms.c
> > ===================================================================
> > --- linux-2.6.x/kernel/kallsyms.c (revision 4212)
> > +++ linux-2.6.x/kernel/kallsyms.c (working copy)
> > @@ -42,7 +42,8 @@
> >
> > static inline int is_kernel_inittext(unsigned long addr)
> > {
> > - if (addr >= (unsigned long)_sinittext
> > + if (system_state == SYSTEM_BOOTING
> > + && addr >= (unsigned long)_sinittext
> > && addr <= (unsigned long)_einittext)
> > return 1;
> > return 0;
> > Index: linux-2.6.x/kernel/module.c
> > ===================================================================
> > --- linux-2.6.x/kernel/module.c (revision 4212)
> > +++ linux-2.6.x/kernel/module.c (working copy)
> > @@ -2121,7 +2121,8 @@
> > struct module *mod;
> >
> > list_for_each_entry(mod, &modules, list) {
> > - if (within(addr, mod->module_init, mod->init_size)
> > + if ((within(addr, mod->module_init, mod->init_size)
> > + && mod->state == MODULE_STATE_COMING)
> > || within(addr, mod->module_core, mod->core_size)) {
> > if (modname)
> > *modname = mod->name;
>
> Both of the above additions could do with a comment explaining what's going
> on.
>
> The first one perhaps should use the more specific initmem_now_dynamic
> which is added by
> gregkh-driver-warn-when-statically-allocated-kobjects-are-used.patch from
> Greg's driver tree. If the intent is to merge that - if not perhaps it can
> be split up.
The intent wasn't to merge that, as it seemed like a big hack. But if
people don't mind, I have no objection to it going in.
thanks,
greg k-h