netdev - Re: [patch 1/1] genirq: Disable interrupts for force threaded handlers

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:   Wed, 17 Mar 2021 17:23:39 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-serial@...r.kernel.org
Subject: Re: [patch 1/1] genirq: Disable interrupts for force threaded
 handlers

On Wed, Mar 17, 2021 at 03:48:06PM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-03-17 15:38:52 [+0100], Thomas Gleixner wrote:
> > With interrupt force threading all device interrupt handlers are invoked
> > from kernel threads. Contrary to hard interrupt context the invocation only
> > disables bottom halfs, but not interrupts. This was an oversight back then
> > because any code like this will have an issue:
> > 
> > thread(irq_A)
> >   irq_handler(A)
> >     spin_lock(&foo->lock);
> > 
> > interrupt(irq_B)
> >   irq_handler(B)
> >     spin_lock(&foo->lock);
> 
> It will not because both threads will wake_up(thread).

Note that the above says "interrupt(irq_B)" suggesting it's a
non-threaded interrupt unlike irq_A.

> It is an issue if
> - if &foo->lock is shared between a hrtimer and threaded-IRQ
> - if &foo->lock is shared between a non-threaded and thread-IRQ

So this is the above case.

> - if &foo->lock is shared between a printk() in hardirq context and
>   thread-IRQ as I learned today.

But generally it's any lock taken by a threaded handler that can end up
being taken in hard interrupt context.

> > This has been triggered with networking (NAPI vs. hrtimers) and console
> > drivers where printk() happens from an interrupt which interrupted the
> > force threaded handler.
> > 
> > Now people noticed and started to change the spin_lock() in the handler to
> > spin_lock_irqsave() which affects performance or add IRQF_NOTHREAD to the
> > interrupt request which in turn breaks RT.
> >
> > Fix the root cause and not the symptom and disable interrupts before
> > invoking the force threaded handler which preserves the regular semantics
> > and the usefulness of the interrupt force threading as a general debugging
> > tool.
> > 
> > For not RT this is not changing much, except that during the execution of
> > the threaded handler interrupts are delayed until the handler
> > returns. Vs. scheduling and softirq processing there is no difference.
> > 
> > For RT kernels there is no issue.
> 
> Acked-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>

Reviewed-by: Johan Hovold <johan@...nel.org>

> > Fixes: 8d32a307e4fa ("genirq: Provide forced interrupt threading")
> > Reported-by: Johan Hovold <johan@...nel.org>
> > Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Eric Dumazet <edumazet@...gle.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> > Cc: netdev <netdev@...r.kernel.org>
> > Cc: "David S. Miller" <davem@...emloft.net>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: Andy Shevchenko <andy.shevchenko@...il.com>
> > CC: Peter Zijlstra <peterz@...radead.org>
> > Cc: linux-serial@...r.kernel.org
> > Cc: netdev <netdev@...r.kernel.org>
> > ---
> >  kernel/irq/manage.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -1142,11 +1142,15 @@ irq_forced_thread_fn(struct irq_desc *de
> >  	irqreturn_t ret;
> >  
> >  	local_bh_disable();
> > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> > +		local_irq_disable();
> >  	ret = action->thread_fn(action->irq, action->dev_id);
> >  	if (ret == IRQ_HANDLED)
> >  		atomic_inc(&desc->threads_handled);
> >  
> >  	irq_finalize_oneshot(desc, action);
> > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> > +		local_irq_enable();
> >  	local_bh_enable();
> >  	return ret;
> >  }

Johan