PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Robin Getz]

From: Robin Getz <rgetz@blackfin.uclinux.org>

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 <rgetz@blackfin.uclinux.org>

---

 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;

Re: PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Andrew Morton]

On Tue, 4 Mar 2008 18:47:15 -0500
Robin Getz <rgetz@blackfin.uclinux.org> wrote:

> From: Robin Getz <rgetz@blackfin.uclinux.org>
> 
> 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 <rgetz@blackfin.uclinux.org>
> 
> ---
> 
>  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.

Re: PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Greg KH]

On Tue, Mar 04, 2008 at 04:05:01PM -0800, Andrew Morton wrote:
> On Tue, 4 Mar 2008 18:47:15 -0500
> Robin Getz <rgetz@blackfin.uclinux.org> wrote:
> 
> > From: Robin Getz <rgetz@blackfin.uclinux.org>
> > 
> > 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 <rgetz@blackfin.uclinux.org>
> > 
> > ---
> > 
> >  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

Re: PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Rusty Russell]

On Wednesday 05 March 2008 10:47:15 Robin Getz wrote:
> From: Robin Getz <rgetz@blackfin.uclinux.org>
>
> 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.

Re: PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Robin Getz]

On Tue 4 Mar 2008 20:43, Rusty Russell pondered:
> On Wednesday 05 March 2008 10:47:15 Robin Getz wrote:
> > From: Robin Getz <rgetz@blackfin.uclinux.org>
> >
> > 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. 

Re: PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Robin Getz]

On Tue 4 Mar 2008 20:43, Rusty Russell pondered:
>     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...

OK since this took so long, I went for bonus points...

The only think I wasn't sure about - is since it now does things in extable.c,
it removes the static inline functions from kallsyms.c - which could cause a
potential slowdown...

I'm sure this will not apply to the mm tree, (it was based on 2.6.24.3), but
should provide some idea for feedback.

Index: linux-2.6.x/kernel/extable.c
===================================================================
--- linux-2.6.x/kernel/extable.c	(revision 4461)
+++ linux-2.6.x/kernel/extable.c	(working copy)
@@ -40,17 +40,40 @@
 	return e;
 }
 
-int core_kernel_text(unsigned long addr)
+int core_kernel_inittext(unsigned long addr)
 {
+	if (addr >= (unsigned long)_sinittext &&
+	    addr <= (unsigned long)_einittext)
+		return 1;
+	return 0;
+}
+
+int core_kernel_extratext(unsigned long addr)
+{
+	if (addr >= (unsigned long)_sextratext &&
+	    addr <= (unsigned long)_eextratext)
+		return 1;
+	return 0;
+}
+
+int __core_kernel_text(unsigned long addr)
+{
 	if (addr >= (unsigned long)_stext &&
 	    addr <= (unsigned long)_etext)
 		return 1;
 
-	if (addr >= (unsigned long)_sinittext &&
-	    addr <= (unsigned long)_einittext)
 		return 1;
 	return 0;
+
 }
+int core_kernel_text(unsigned long addr)
+{
+	return __core_kernel_text(addr) || core_kernel_inittext(addr);
+}
 
 int __kernel_text_address(unsigned long addr)
 {
@@ -65,3 +88,4 @@
 		return 1;
 	return module_text_address(addr) != NULL;
 }
+
Index: linux-2.6.x/kernel/kallsyms.c
===================================================================
--- linux-2.6.x/kernel/kallsyms.c	(revision 4461)
+++ linux-2.6.x/kernel/kallsyms.c	(working copy)
@@ -45,47 +45,24 @@
 
 extern const unsigned long kallsyms_markers[] __attribute__((weak));
 
-static inline int is_kernel_inittext(unsigned long addr)
-{
-	if (addr >= (unsigned long)_sinittext
-	    && addr <= (unsigned long)_einittext)
-		return 1;
-	return 0;
-}
-
-static inline int is_kernel_extratext(unsigned long addr)
-{
-	if (addr >= (unsigned long)_sextratext
-	    && addr <= (unsigned long)_eextratext)
-		return 1;
-	return 0;
-}
-
 static inline int is_kernel_text(unsigned long addr)
 {
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_etext)
+	if (core_kernel_text(addr))
 		return 1;
-	return in_gate_area_no_task(addr);
-}
 
-static inline int is_kernel(unsigned long addr)
-{
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
-		return 1;
 	return in_gate_area_no_task(addr);
 }
 
 static int is_ksym_addr(unsigned long addr)
 {
-	if (all_var)
-		return is_kernel(addr);
+	if (all_var) {
+		if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
+			return 1;
+		else
+			return in_gate_area_no_task(addr);
+	}
 
-	return is_kernel_text(addr) || is_kernel_inittext(addr) ||
-		is_kernel_extratext(addr);
+	return is_kernel_text(addr) || core_kernel_extratext(addr);
 }
 
 /* expand a compressed symbol data into the resulting uncompressed string,
@@ -215,7 +192,7 @@
 
 	/* if we found no next symbol, we use the end of the section */
 	if (!symbol_end) {
-		if (is_kernel_inittext(addr))
+		if (core_kernel_inittext(addr))
 			symbol_end = (unsigned long)_einittext;
 		else if (all_var)
 			symbol_end = (unsigned long)_end;
Index: linux-2.6.x/include/linux/kernel.h
===================================================================
--- linux-2.6.x/include/linux/kernel.h	(revision 4461)
+++ linux-2.6.x/include/linux/kernel.h	(working copy)
@@ -167,6 +167,8 @@
 extern unsigned long long memparse(char *ptr, char **retptr);
 
 extern int core_kernel_text(unsigned long addr);
+extern int core_kernel_extratext(unsigned long addr);
+extern int core_kernel_inittext(unsigned long addr);
 extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
 struct pid;

Re: PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Rusty Russell]

On Thursday 03 April 2008 08:01:25 Robin Getz wrote:
> On Tue 4 Mar 2008 20:43, Rusty Russell pondered:
> >     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...
>
> OK since this took so long, I went for bonus points...

Looks good.  I don't have extratext in my kernel tho 
(v2.6.25-rc8-139-ge315c12).  Otherwise I'd suspect it should be consistently 
used between extable and kallsyms.

Thanks!
Rusty.

Re: PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Robin Getz]

On Thu 3 Apr 2008 19:41, Rusty Russell pondered:
> On Thursday 03 April 2008 08:01:25 Robin Getz wrote:
> > On Tue 4 Mar 2008 20:43, Rusty Russell pondered:
> > >     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...
> >
> > OK since this took so long, I went for bonus points...
> 
> Looks good.  I don't have extratext in my kernel tho 
> (v2.6.25-rc8-139-ge315c12). 

OK - for some reason - I tested the patch, sent it upstream, but it wasn't 
kept in our local version when we updated to 2.6.24 :(

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=a3b81113fb6658629f4ebaabf8dd3067cd341020

I'll update and send something new.

> Otherwise I'd suspect it should be consistently used between extable 
> and kallsyms. 

Re: PATCH [1/1]: Don't return symbol lables in init sections after they have been freed

[Robin Getz]

On Thu 3 Apr 2008 19:41, Rusty Russell pondered:
> On Thursday 03 April 2008 08:01:25 Robin Getz wrote:
> > On Tue 4 Mar 2008 20:43, Rusty Russell pondered:
> > >     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...
> >
> > OK since this took so long, I went for bonus points...
> 
> Looks good.  I don't have extratext in my kernel tho 
> (v2.6.25-rc8-139-ge315c12).  Otherwise I'd suspect it should be consistently 
> used between extable and kallsyms.

update patch - refactored a bit more - all address checking is removed from
kallsyms, and moved to extable - if this looks ok - I will send it properly...


Index: linux-2.6.x/kernel/kallsyms.c
===================================================================
--- linux-2.6.x/kernel/kallsyms.c	(revision 4546)
+++ linux-2.6.x/kernel/kallsyms.c	(working copy)
@@ -45,38 +45,20 @@
 
 extern const unsigned long kallsyms_markers[] __attribute__((weak));
 
-static inline int is_kernel_inittext(unsigned long addr)
+static int is_ksym_addr(unsigned long addr)
 {
-	if (addr >= (unsigned long)_sinittext
-	    && addr <= (unsigned long)_einittext)
-		return 1;
-	return 0;
-}
+	if (all_var) {
+		if (core_kernel_anywhere(addr))
+			return 1;
 
-static inline int is_kernel_text(unsigned long addr)
-{
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_etext)
-		return 1;
-	return in_gate_area_no_task(addr);
-}
+		return in_gate_area_no_task(addr);
+	}
 
-static inline int is_kernel(unsigned long addr)
-{
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
+	if (core_kernel_text(addr))
 		return 1;
+
 	return in_gate_area_no_task(addr);
-}
 
-static int is_ksym_addr(unsigned long addr)
-{
-	if (all_var)
-		return is_kernel(addr);
-
-	return is_kernel_text(addr) || is_kernel_inittext(addr);
 }
 
 /* expand a compressed symbol data into the resulting uncompressed string,
@@ -206,7 +188,7 @@
 
 	/* if we found no next symbol, we use the end of the section */
 	if (!symbol_end) {
-		if (is_kernel_inittext(addr))
+		if (core_kernel_inittext(addr))
 			symbol_end = (unsigned long)_einittext;
 		else if (all_var)
 			symbol_end = (unsigned long)_end;
Index: linux-2.6.x/kernel/extable.c
===================================================================
--- linux-2.6.x/kernel/extable.c	(revision 4541)
+++ linux-2.6.x/kernel/extable.c	(working copy)
@@ -40,18 +40,43 @@
 	return e;
 }
 
-int core_kernel_text(unsigned long addr)
+int core_kernel_inittext(unsigned long addr)
 {
+	if (addr >= (unsigned long)_sinittext &&
+	    addr <= (unsigned long)_einittext)
+		return 1;
+	return 0;
+}
+
+#ifdef CONFIG_KALLSYMS_ALL
+int core_kernel_anywhere(unsigned long addr)
+{
+	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
+		return 1;
+	return 0;
+}
+#endif
+
+int __core_kernel_text(unsigned long addr)
+{
 	if (addr >= (unsigned long)_stext &&
 	    addr <= (unsigned long)_etext)
 		return 1;
 
-	if (addr >= (unsigned long)_sinittext &&
-	    addr <= (unsigned long)_einittext)
- 		return 1;

 	return 0;
 }
 
+int core_kernel_text(unsigned long addr)
+{
+	return __core_kernel_text(addr) || core_kernel_inittext(addr);
+}
+
 int __kernel_text_address(unsigned long addr)
 {
 	if (core_kernel_text(addr))
@@ -65,3 +90,4 @@
 		return 1;
 	return module_text_address(addr) != NULL;
 }
+
Index: linux-2.6.x/include/linux/kernel.h
===================================================================
--- linux-2.6.x/include/linux/kernel.h	(revision 4541)
+++ linux-2.6.x/include/linux/kernel.h	(working copy)
@@ -167,6 +167,8 @@
 extern unsigned long long memparse(char *ptr, char **retptr);
 
 extern int core_kernel_text(unsigned long addr);
+extern int core_kernel_inittext(unsigned long addr);
+extern int core_kernel_anywhere(unsigned long addr);
 extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
 struct pid;