Messages in this thread | | | Subject | Re: [PATCH 1/2] genirq: introduce update_irq_devid() | From | luoben <> | Date | Mon, 12 Aug 2019 10:59:48 +0800 |
| |
在 2019/8/9 上午3:56, Thomas Gleixner 写道: > On Thu, 8 Aug 2019, Ben Luo wrote: >> +int update_irq_devid(unsigned int irq, void *dev_id, void *new_dev_id) >> +{ >> + struct irq_desc *desc = irq_to_desc(irq); >> + struct irqaction *action, **action_ptr; >> + unsigned long flags; >> + >> + WARN(in_interrupt(), >> + "Trying to update IRQ %d from IRQ context!\n", irq); > This is broken. The function needs to return on that condition. Actually it > cannot even be called from non-preemptible code. > > What's worse is that if the interrupt in question is handled concurrently, > then it will either see the old or the new dev_id and because the interrupt > handler loop runs with desc->lock dropped even more crap can happen because > dev_id can be subject to load and store tearing. > > Staring at that, I see that there is the same issue in setup_irq() and > free_irq(). It's actually worse there. I'll have a look.
ok, will return with an error code in v2 in this case
> >> + /* >> + * There can be multiple actions per IRQ descriptor, find the right >> + * one based on the dev_id: >> + */ >> + action_ptr = &desc->action; >> + for (;;) { >> + action = *action_ptr; >> + >> + if (!action) { >> + WARN(1, "Trying to update already-free IRQ %d\n", irq); > That's wrong in two aspects: > > 1) The warn should be outside of the locked region. > > 2) Just having the irq number is not useful for debugging either > when the interrupt is shared. will take care in v2 >> + raw_spin_unlock_irqrestore(&desc->lock, flags); >> + chip_bus_sync_unlock(desc); >> + return -ENXIO; >> + } >> + >> + if (action->dev_id == dev_id) { >> + action->dev_id = new_dev_id; >> + break; >> + } >> + action_ptr = &action->next; >> + } >> + >> + raw_spin_unlock_irqrestore(&desc->lock, flags); >> + chip_bus_sync_unlock(desc); >> + >> + /* >> + * Make sure it's not being used on another CPU: >> + * There is a risk of UAF for old *dev_id, if it is >> + * freed in a short time after this func returns >> + */ >> + synchronize_irq(irq); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(update_irq_devid); > EXPORT_SYMBOL_GPL() please. thanks, will use EXPORT_SYMBOL_GPL in v2 > > Thanks, > > tglx
Thanks,
Ben
| |