linux-kernel - Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 Jul 2018 00:36:57 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Bjorn Helgaas <helgaas@...nel.org>
cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        Christoph Hellwig <hch@....de>,
        LKML <linux-kernel@...r.kernel.org>,
        Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH] PCI: let pci_request_irq properly deal with threaded
 interrupts

On Mon, 30 Jul 2018, Bjorn Helgaas wrote:

> [+cc Thomas, Christoph, LKML]

+ Marc

> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> > If we have a threaded interrupt with the handler being NULL, then
> > request_threaded_irq() -> __setup_irq() will complain and bail out
> > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> > being NULL and set IRQF_ONESHOT in this case.
> > 
> > This change is needed to migrate the mei_me driver to
> > pci_alloc_irq_vectors() and pci_request_irq().
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> 
> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> usage isn't mentioned in the request_threaded_irq() function doc or
> Documentation/

Right. The documentation really needs some love and care. :(

Yes, request for pure threaded interrupts are rejected if the oneshot flag
is not set. The reason is that this would be deadly especially with level
triggered interrupts because the primary default handler just wakes the
thread and then reenables interrupts, which will make the interrupt come
back immediately and the thread won't have a chance to actually shut it up
in the device.

That made me look into that code again and I found that we added a flag for
irq chips to tell the core that the interrupt is one shot safe, i.e. that
it can be requested w/o IRQF_ONESHOT. That was initially added to optimize
MSI based interrupts which are oneshot safe by implementation.

  dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe")

The original patch added that flag to the x86 MSI irqchip code, but that
part was not applied for reasons which slipped from memory. It might be
worthwhile to revisit that in order to avoid the mask/unmask overhead for
such cases.

Anyway for the patch in question:

       Reviewed-by: Thomas Gleixner <tglx@...utronix.de>

Thanks,

	tglx