linux-kernel - Re: [PATCH v3] genirq: Check irq disabled & masked states in irq

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:   Mon, 26 Jun 2017 14:22:51 +0800
From:   jeffy <jeffy.chen@...k-chips.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     Tomasz Figa <tfiga@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Brian Norris <briannorris@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH v3] genirq: Check irq disabled & masked states in irq_shutdown

Hi Thomas Gleixner,

On 05/31/2017 07:02 AM, Thomas Gleixner wrote:
> On Mon, 29 May 2017, jeffy.chen wrote:
>> i think if we want to make all irq enable/disable balance, maybe we can:
>>
>> 1/ only call irq_enable/disable from enable/disable_irq(change other
>> irq_enable/disable to enable/disable_irq), so they would be protected by the
>> refcnt(deph)
>
> You cannot call enable/disable_irq() from places which call
> irq_enable/disable() due to locking reasons.
>
> __disable_irq()/__enable_irq() can/must be called with desc->lock held, but
> __enable_irq() does more than just calling irq_enable().
>
> So no, it's not that simple.
>
>> 2/ disable lazy stuff in irq_shoutdown(we already did this in free_irq)
>
> No, irq_shutdown() is called from other places as well.
>
>> 3/ in irq_shutdown, set depth to 0 if it's not disabled and masked(for lazy stuff)
>> before calling disable_irq,
>
> Uurgh, no. That's a unholy hack.
>
> So what should be done to fix this is to make consequent use of the state bits.
>
>       irq_disable()
>          if (irqd_irq_disabled())
> 	   return;
> 	irqd_set_irq_disabled();
> 	....
>
> This should be done for both mask/unmask disable/enable. You get the idea.
ok
>
> We probably want a third state bit for STARTED_UP and do the same dance in
> startup/shutdown as well. Which brings me to a different issue, which is
> outside the scope of your problem, but looking at the code made me find it.
>
> If a interrupt is marked IRQ_NOAUTOEN then request_irq() will not invoke
> irq_startup(). The interrupt is just completely set up, but stays disabled.
> It is enabled later via enable_irq(). That works so far with no complaints,
> but there is an interesting twist:
>
> In that NOAUTOEN case nothing ever calls irq_startup() on that irq, which
> means that in case the irq_chip has a irq_startup() callback nothing will
> invoke it and also irq_domain_activate_irq() will never be invoked on such
> an irq.
>
> Looks like all implementations which use IRQ_NOAUTOEN are not sensitive to
> that. It's been broken forever.
>
> Fixing this needs the extra state bit IRQD_ STARTED_UP as we cannot reuse
> the IRQD_ACTIVATED bit because some of the interrupts are actually
> activated before they are requested.
>
> Too tired to fix that now, but I'll have a look tomorrow. Once this is
> fixed, you can add the extra bits to prevent this disable/enable calls
> which cause the imbalance deeper down.
i saw your patches landed, so i sent a patch for 
enable/disable/unmask/mask_irq, please help to review :)
>
> Thanks,
>
> 	tglx
>
>
>
>
>
>
>
>
>
>
>
>