[PATCH] NULL struct irq_desc's member 'name' in dynamic_irq_cleanup()

[PATCH] NULL struct irq_desc's member 'name' in dynamic_irq_cleanup()

[Dean Nelson]

If the member 'name' of the irq_desc structure happens to point to a character
string that is resident within a kernel module, problems insue if that module
is rmmod'd (at which time dynamic_irq_cleanup() is called) and then later
show_interrupts() is called by someone. It is also not a good thing if the
character string resided in kmalloc'd space that has been kfree'd (after
having called dynamic_irq_cleanup()). dynamic_irq_cleanup() fails to NULL
the 'name' member and show_interrupts() references it on a few architectures
(like h8300, sh and x86).

Signed-off-by: Dean Nelson <dcn@sgi.com>

---

 kernel/irq/chip.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux/kernel/irq/chip.c
===================================================================
--- linux.orig/kernel/irq/chip.c	2008-10-15 07:44:31.000000000 -0500
+++ linux/kernel/irq/chip.c	2008-10-16 06:55:56.000000000 -0500
@@ -79,6 +79,7 @@ void dynamic_irq_cleanup(unsigned int ir
 	desc->chip_data = NULL;
 	desc->handle_irq = handle_bad_irq;
 	desc->chip = &no_irq_chip;
+	desc->name = NULL;
 	spin_unlock_irqrestore(&desc->lock, flags);
 }
 

Re: [PATCH] NULL struct irq_desc's member 'name' in dynamic_irq_cleanup()

[Andrew Morton]

On Thu, 16 Oct 2008 07:58:08 -0500
Dean Nelson <dcn@sgi.com> wrote:

> If the member 'name' of the irq_desc structure happens to point to a character
> string that is resident within a kernel module, problems insue if that module
> is rmmod'd (at which time dynamic_irq_cleanup() is called) and then later
> show_interrupts() is called by someone.

It would be nice to spell out what the "problems" are.

> It is also not a good thing if the
> character string resided in kmalloc'd space that has been kfree'd (after
> having called dynamic_irq_cleanup()). dynamic_irq_cleanup() fails to NULL
> the 'name' member and show_interrupts() references it on a few architectures
> (like h8300, sh and x86).
> 
> Signed-off-by: Dean Nelson <dcn@sgi.com>
> 
> ---
> 
>  kernel/irq/chip.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: linux/kernel/irq/chip.c
> ===================================================================
> --- linux.orig/kernel/irq/chip.c	2008-10-15 07:44:31.000000000 -0500
> +++ linux/kernel/irq/chip.c	2008-10-16 06:55:56.000000000 -0500
> @@ -79,6 +79,7 @@ void dynamic_irq_cleanup(unsigned int ir
>  	desc->chip_data = NULL;
>  	desc->handle_irq = handle_bad_irq;
>  	desc->chip = &no_irq_chip;
> +	desc->name = NULL;
>  	spin_unlock_irqrestore(&desc->lock, flags);
>  }
>  

Because we should work out whether this should be backported into
-stable.  And if so, how far back it should go.

Re: [PATCH] NULL struct irq_desc's member 'name' in dynamic_irq_cleanup()

[Dean Nelson]

On Thu, Oct 16, 2008 at 04:11:45PM -0700, Andrew Morton wrote:
> On Thu, 16 Oct 2008 07:58:08 -0500
> Dean Nelson <dcn@sgi.com> wrote:
> 
> > If the member 'name' of the irq_desc structure happens to point to a character
> > string that is resident within a kernel module, problems insue if that module
> > is rmmod'd (at which time dynamic_irq_cleanup() is called) and then later
> > show_interrupts() is called by someone.
> 
> It would be nice to spell out what the "problems" are.

I'm working on a drivers/misc/sgi-xp patchset for SGI's UV system that will
call uv_setup_irq() and uv_teardown_irq(), both of which were submitted as a
patch (Subject: [PATCH] x86, UV: add uv_setup_irq() and uv_teardown_irq()
functions v.3) which has been applied to Ingo's tip/x86/uv.

The first argument to uv_setup_irq() is a pointer to the irq_name to be
stored in irq_desc.name. For XPC, it points to a string resident in the
kernel module.

When I rmmod xpc, uv_teardown_irq() calls destroy_irq() which in turn
calls dynamic_irq_cleanup().

In arch/x86/kernel/irq_64.c, show_interrupts() references the 'name' member
in the following manner:

	seq_printf(p, "-%-8s", irq_desc[i].name);

In my case, show_interrupts was called by the irq balancer and the end result
was an Oops. Note that this can occur later than the rmmod such that the
two events do not appear related.

> 
> Because we should work out whether this should be backported into
> -stable.  And if so, how far back it should go.

I don't have an answer for this.