Re: [RFC][PATCH] IRQ: Fix oneshot irq race between irq_finalize_oneshotand handle_level_irq

From: Thomas Gleixner
Date: Tue Mar 09 2010 - 18:22:31 EST


B1;2005;0cOn Tue, 9 Mar 2010, Lars-Peter Clausen wrote:
>
> - desc->status |= IRQ_INPROGRESS;
> + desc->status |= IRQ_INPROGRESS | IRQ_ONESHOT_INPROGRESS;
> raw_spin_unlock(&desc->lock);

That keeps the IRQ_ONESHOT_INPROGRESS dangling for non ONESHOT
interrupts. Not a big deal, but not pretty either.

The race between the thread and the irq handler exists indeed on SMP,
but I think there are more fundamental issues about the state which
need to be addressed.

The first thing is that we do not mark the status MASKED when we
actually mask the interrupt in mask_ack_irq().

That conditional MASKED after running the primary handler is really
horrible - I already ranted in private at the moron who committed that
crime :)

So the following patch fixes that and the SMP race scenario:

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index d70394f..3e53334 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -359,6 +359,7 @@ static inline void mask_ack_irq(struct irq_desc *desc, int irq)
if (desc->chip->ack)
desc->chip->ack(irq);
}
+ desc->status |= IRQ_MASKED;
}

/*
@@ -484,10 +485,11 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc)
raw_spin_lock(&desc->lock);
desc->status &= ~IRQ_INPROGRESS;

- if (unlikely(desc->status & IRQ_ONESHOT))
- desc->status |= IRQ_MASKED;
- else if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+ if (!(desc->status & (IRQ_DISABLED | IRQ_ONESHOT)) &&
+ desc->chip->unmask) {
desc->chip->unmask(irq);
+ desc->status &= ~IRQ_MASKED;
+ }
out_unlock:
raw_spin_unlock(&desc->lock);
}

But that change opens up another race which is similar to the one you
pointed out:

CPU0 CPU1
hande_level_irq(irq X)
mask_ack_irq(irq X)
handle_IRQ_event(irq X)
wake_up(thread_handler)
thread handler(irq X) runs
interrupt irqY
handle_*_irq(irq Y)
.....
finalize_oneshot(irq X)
unmask(irq X)
interrupt irq X
handle_level_irq(irq X)
mask_ack_irq(irq X)
return from irq X due to IRQ_INPROGRESS

return from irq Y

return from irq X w/o unmask due to IRQ_ONESHOT

That's pretty unlikely, but possible. I know that your patch would
mitigate that problem, but I'm not happy about making it go away with
non obvious state magic. I think I know how to fix it, but I need some
sleep now.

Thanks,

tglx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/