linux-kernel - Re: [2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared 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]

Message-ID: <2582391.E0BmZJ6Wil@np-p-burton>
Date:   Wed, 6 Sep 2017 07:01:54 -0700
From:   Paul Burton <paul.burton@...tec.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     LKML <linux-kernel@...r.kernel.org>,
        jeffy <jeffy.chen@...k-chips.com>,
        Brian Norris <briannorris@...omium.org>,
        Marc Zyngier <marc.zyngier@....com>, <dianders@...omium.org>,
        <tfiga@...omium.org>, <james.hogan@...tec.com>
Subject: Re: [2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts

Hi Thomas,

On Wednesday, 6 September 2017 01:16:48 PDT Thomas Gleixner wrote:
> On Tue, 5 Sep 2017, Paul Burton wrote:
> > I'm currently attempting to clean up a hack that we have in the MIPS GIC
> > irqchip driver - we have some interrupts which are really per-CPU, but are
> > currently used with the regular non-per-CPU IRQ APIs. Please search for
> > usage of gic_all_vpes_local_irq_controller (or for the string "HACK") in
> > drivers/ irqchip/irq-mips-gic.c if you wish to find what I'm talking
> > about. The important details are that the interrupts in question are both
> > per-CPU and on many systems are shared (between the CPU timer,
> > performance counters & fast debug channel).
> > 
> > I have been attempting to move towards using the per-CPU APIs instead in
> > order to remove this hack - ie. using setup_percpu_irq() &
> > enable_percpu_irq() in> 
> > place of plain old setup_irq(). Unfortunately what I've run into is this:
> >   - Per-CPU interrupts get the IRQ_NOAUTOEN flag set by default, in
> >   
> >     irq_set_percpu_devid_flags(). I can see why this makes sense in the
> >     general case, since the alternative is setup_percpu_irq() enabling the
> >     interrupt on the CPU that calls it & leaving it disabled on others,
> >     which
> >     feels a little unclean.
> >   
> >   - Your warning above triggers when a shared interrupt has the
> >   IRQ_NOAUTOEN
> >   
> >     flag set. I can see why your warning makes sense if another driver has
> >     already enabled the shared interrupt, which would make IRQ_NOAUTOEN
> >     ineffective. I'm not sure I follow your comment above the warning
> >     though -
> >     it sounds like you're trying to describe something else?
> > > 
> > > +			/*
> > > +			 * Shared interrupts do not go well with disabling
> > > +			 * auto enable. The sharing interrupt might request
> > > +			 * it while it's still disabled and then wait for
> > > +			 * interrupts forever.
> > > +			 */
> 
> Assume the following:
> 
>        request_irq(X, handler1, NOAUTOEN|SHARED, dev1);
> 
> now the second device does:
> 
>        request_irq(X, handler2, SHARED, dev2):
> 
> which will see the first handler installed, so it wont run into the code
> path which starts up the interrupt. That means as long as dev1 does not
> explicitely enable the interrupt dev2 will wait for it forever.

Ok, makes sense.

> > For my interrupts which are both per-CPU & shared the combination of these
> > 2> 
> > facts mean I end up triggering your warning. My current ideas include:
> >   - I could clear the IRQ_NOAUTOEN flag before calling setup_percpu_irq().
> >   In
> >   
> >     my cases that should be fine - we call enable_percpu_irq() anyway, and
> >     would just enable the IRQ slightly earlier on the CPU which calls
> >     setup_percpu_irq() which wouldn't be a problem. It feels a bit yucky
> >     though.
> 
> What's the problem with IRQ_NOAUTOEN and do
> 
>        setup_percpu_irq();
>        enable_percpu_irq();
> 
> on the boot CPU and then later call it when the secondary CPUs come up in
> cpu bringup code or a hotplug state callback?

There's no problem with that at all, apart from that it triggers your warning 
when the boot CPU calls setup_percpu_irq().

Thanks,
    Paul
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)