linux-kernel - Re: handle_bad

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, 15 May 2017 01:46:31 -0700
From:   Gregory Fong <gregory.0xf0@...il.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux@...linux.org.uk, Florian Fainelli <f.fainelli@...il.com>
Subject: Re: handle_bad_irq and locking

On Fri, May 12, 2017 at 1:40 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Thu, 11 May 2017, Gregory Fong wrote:
>> Hi Thomas,
>>
>> I noticed that when you changed arm irq handling to use the generic
>> implementation back in 2006 that you changed do_bad_IRQ() to the
>> following:
>>
>> +#define do_bad_IRQ(irq,desc,regs)                      \
>> +do {                                                   \
>> +       spin_lock(&desc->lock);                         \
>> +       handle_bad_irq(irq, desc, regs);                \
>> +       spin_unlock(&desc->lock);                       \
>> +} while(0)
>>
>> and it's mostly stayed the same since then.  As such, there are a few
>> examples of this being open-coded in the kernel in various irqchip
>> handlers such as that of drivers/irqchip/irq-brcmstb-l2.c, and even
>> more cases where the lock isn't being grabbed before calling
>> handle_bad_irq().
>>
>> Since handle_bad_irq() is sort of a flow handler like
>> handle_edge_irq() etc., do you think it would make sense to do the
>> same as those and have it do the locking on its own?  A quick look
>> through existing users of handle_bad_irq() at [1] suggests that either
>> the locking isn't necessary or it's almost always done wrong.
>
> Right. So the proper solution to this is to create a generic function
>
>        handle_bad_irq_desc()
>
> have proper locking in it and move all users (including do_bad_IRQ()) over
> to it.

I'm guessing the different name would be to avoid breaking existing
users.  Wouldn't it be better to just add locking in handle_bad_irq(),
simultaneously removing the lock/unlock around in its invocations in
the few places where that was actually done?  There aren't any
existing builtin IRQ flow handlers that end in "_desc".

Otherwise I'll move forward with implementing handle_bad_irq_desc()
and updating existing users.

Thanks,
Gregory