linux-kernel - Re: [PATCH] set_irq_noprobe shouldn't be _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 9 Jan 2010 15:17:24 +0100
From:	Jean Delvare <khali@...ux-fr.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...e.hu>, Yinghai Lu <yinghai@...nel.org>
Subject: Re: [PATCH] set_irq_noprobe shouldn't be __init

Hi Andrew,

On Fri, 8 Jan 2010 16:39:10 -0800, Andrew Morton wrote:
> On Tue, 5 Jan 2010 20:38:42 +0100
> Jean Delvare <khali@...ux-fr.org> wrote:
> 
> > Non-__init functions need to call set_irq_noprobe() so this function
> > shouldn't be marked __init. Also remove __init from set_irq_probe()
> > for consistency.
> > 
> > Signed-off-by: Jean Delvare <khali@...ux-fr.org>
> > Cc: Ralf Baechle <ralf@...ux-mips.org>
> > Cc: Samuel Ortiz <sameo@...ux.intel.com>
> > ---
> >  kernel/irq/chip.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > --- linux-2.6.33-rc2.orig/kernel/irq/chip.c	2009-12-18 08:34:52.000000000 +0100
> > +++ linux-2.6.33-rc2/kernel/irq/chip.c	2010-01-05 17:21:38.000000000 +0100
> > @@ -682,7 +682,7 @@ set_irq_chip_and_handler_name(unsigned i
> >  	__set_irq_handler(irq, handle, 0, name);
> >  }
> >  
> > -void __init set_irq_noprobe(unsigned int irq)
> > +void set_irq_noprobe(unsigned int irq)
> >  {
> >  	struct irq_desc *desc = irq_to_desc(irq);
> >  	unsigned long flags;
> > @@ -697,7 +697,7 @@ void __init set_irq_noprobe(unsigned int
> >  	raw_spin_unlock_irqrestore(&desc->lock, flags);
> >  }
> >  
> > -void __init set_irq_probe(unsigned int irq)
> > +void set_irq_probe(unsigned int irq)
> >  {
> >  	struct irq_desc *desc = irq_to_desc(irq);
> >  	unsigned long flags;
> 
> These non-__init callers will instantly crash, surely?  Are there any
> reports of such crashes?

I am not aware of such crashes being reported, but I am not the
maintainer of the affected drivers (these are mfd drivers.) I suppose
that the crash would only happen if these drivers were built as
modules: if built into the kernel (and they always are, being boolean
options in Kconfig), maybe they have time to initialize before the
__init code is removed? I'm not familiar enough with this mechanism to
tell.

The only thing I'm sure of is that 'make
CONFIG_DEBUG_SECTION_MISMATCH=y' complains.

I initially proposed the most immediate fix, but it might as well be
that it is reasonable to leave set_irq_probe() in __init and fix all
callers, under the assumption that all drivers which need it will be
for specific chips which require built-in support and can't be
hot-plugged by design.

What I don't quite get is how to guarantee that the devices in question
will be reachable early. These are SPI and I2C devices, so they
require the right bus driver to be loaded first. But this cannot happen
if they are built as modules (which nothing prevents as far as I can
see.) So I suspect that, if everything in the drivers was tagged __init
as it would have to if we want set_irq_probe() in __init, but the
relevant SPI or I2C bus driver was built as a module, bad things would
happen.

The proper fix in that case is somewhat beyond me. This might be one of
these cases where I should have refrained from sending a patch when I
finally did not know what the best solution to the problem would be.
Feel free to ignore me.

> If you're talking about new code then what is the status of that code?
> 
> Right now I don't know if this patch is needed in 2.6.34, 2.6.33 or
> 2.6.32.x and earlier.
> 
> IOW: the changelog sucks :)

You're right, sorry about that. The affected drivers are
twl4030/twl6030, ezx-pcap and wm831x (all mfd drivers), offending lines
are in the kernel tree since 2.6.28/2.6.33, 2.6.31 and 2.6.33,
respectively.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/