threadirqs deadlocks

threadirqs deadlocks

[Johan Hovold]

Hi Thomas,

We've gotten reports of lockdep splats correctly identifying a potential
deadlock in serial drivers when running with forced interrupt threading.

Typically, a serial driver takes the port spin lock in its interrupt
handler, but unless also disabling interrupts the handler can be
preempted by another interrupt which can end up calling printk. The
console code takes then tries to take the port lock and we deadlock.

It seems to me that forced interrupt threading cannot generally work
without updating drivers that expose locks that can be taken by other
interrupt handlers, for example, by using spin_lock_irqsave() in their
interrupt handlers or marking their interrupts as IRQF_NO_THREAD.

What are your thoughts on this given that forced threading isn't that
widely used and was said to be "mostly a debug option". Do we need to
vet all current and future drivers and adapt them for "threadirqs"?

Note that we now have people sending cleanup patches for interrupt
handlers by search-and-replacing spin_lock_irqsave() with spin_lock()
which can end up exposing this more.

Johan

Re: threadirqs deadlocks

[Thomas Gleixner]

Johan,

On Tue, Mar 16 2021 at 11:56, Johan Hovold wrote:
> We've gotten reports of lockdep splats correctly identifying a potential
> deadlock in serial drivers when running with forced interrupt threading.
>
> Typically, a serial driver takes the port spin lock in its interrupt
> handler, but unless also disabling interrupts the handler can be
> preempted by another interrupt which can end up calling printk. The
> console code takes then tries to take the port lock and we deadlock.
>
> It seems to me that forced interrupt threading cannot generally work
> without updating drivers that expose locks that can be taken by other
> interrupt handlers, for example, by using spin_lock_irqsave() in their
> interrupt handlers or marking their interrupts as IRQF_NO_THREAD.

The latter is the worst option because that will break PREEMPT_RT.

> What are your thoughts on this given that forced threading isn't that
> widely used and was said to be "mostly a debug option". Do we need to
> vet all current and future drivers and adapt them for "threadirqs"?
>
> Note that we now have people sending cleanup patches for interrupt
> handlers by search-and-replacing spin_lock_irqsave() with spin_lock()
> which can end up exposing this more.

It's true that for !RT it's primarily a debug option, but occasionaly a
very valuable one because it does not take the whole machine down when
something explodes in an interrupt handler. Used it just a couple of
weeks ago successfully :)

So we have several ways out of that:

  1) Do the lock() -> lock_irqsave() dance

  2) Delay printing from hard interrupt context (which is what RT does)

  3) Actually disable interrupts before calling the force threaded
     handler.

I'd say #3 is the right fix here. It's preserving the !RT semantics
and the usefulness of threadirqs for debugging and spare us dealing with
the script kiddies.

Something like the below.

Thanks,

        tglx
---
--- 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_enable();
 	local_bh_enable();
 	return ret;
 }

Re: threadirqs deadlocks

[Johan Hovold]

On Wed, Mar 17, 2021 at 02:24:04PM +0100, Thomas Gleixner wrote:

> On Tue, Mar 16 2021 at 11:56, Johan Hovold wrote:

> > It seems to me that forced interrupt threading cannot generally work
> > without updating drivers that expose locks that can be taken by other
> > interrupt handlers, for example, by using spin_lock_irqsave() in their
> > interrupt handlers or marking their interrupts as IRQF_NO_THREAD.
> 
> The latter is the worst option because that will break PREEMPT_RT.
> 
> > What are your thoughts on this given that forced threading isn't that
> > widely used and was said to be "mostly a debug option". Do we need to
> > vet all current and future drivers and adapt them for "threadirqs"?
> >
> > Note that we now have people sending cleanup patches for interrupt
> > handlers by search-and-replacing spin_lock_irqsave() with spin_lock()
> > which can end up exposing this more.
> 
> It's true that for !RT it's primarily a debug option, but occasionaly a
> very valuable one because it does not take the whole machine down when
> something explodes in an interrupt handler. Used it just a couple of
> weeks ago successfully :)
> 
> So we have several ways out of that:
> 
>   1) Do the lock() -> lock_irqsave() dance
> 
>   2) Delay printing from hard interrupt context (which is what RT does)

While this is probably mostly an issue for console drivers, the problem
is more general and we'd need to identify and add workarounds for any
lock that could be taken by a second interrupt handler.

>   3) Actually disable interrupts before calling the force threaded
>      handler.
> 
> I'd say #3 is the right fix here. It's preserving the !RT semantics
> and the usefulness of threadirqs for debugging and spare us dealing with
> the script kiddies.

I was hoping you'd say that. :) Just wasn't sure whether it would
cripple threadirqs too much.

> Something like the below.

Looks good to me. Do you want to spin that into a patch or shall I do
it after some testing?

Johan

Re: threadirqs deadlocks

[Thomas Gleixner]

On Wed, Mar 17 2021 at 15:00, Johan Hovold wrote:
> On Wed, Mar 17, 2021 at 02:24:04PM +0100, Thomas Gleixner wrote:
>> Something like the below.
>
> Looks good to me. Do you want to spin that into a patch or shall I do
> it after some testing?

I'll send one in a few