Messages in this thread | | | Subject | Re: [PATCH v3 3/3] uio: fix crash after the device is unregistered | From | Mike Christie <> | Date | Mon, 9 Jul 2018 12:06:32 -0500 |
| |
On 07/06/2018 08:28 PM, Xiubo Li wrote: > On 2018/7/7 2:23, Mike Christie wrote: >> On 07/05/2018 09:57 PM, xiubli@redhat.com wrote: >>> static irqreturn_t uio_interrupt(int irq, void *dev_id) >>> { >>> struct uio_device *idev = (struct uio_device *)dev_id; >>> - irqreturn_t ret = idev->info->handler(irq, idev->info); >>> + irqreturn_t ret; >>> + >>> + mutex_lock(&idev->info_lock); >>> + if (!idev->info) { >>> + ret = IRQ_NONE; >>> + goto out; >>> + } >>> + ret = idev->info->handler(irq, idev->info); >>> if (ret == IRQ_HANDLED) >>> uio_event_notify(idev->info); >>> +out: >>> + mutex_unlock(&idev->info_lock); >>> return ret; >>> } >> >> Do you need the interrupt related changes in this patch and the first >> one? > Actually, the NULL checking is not a must, we can remove this. But the > lock/unlock is needed. >> When we do uio_unregister_device -> free_irq does free_irq return >> when there are no longer running interrupt handlers that we requested? >> >> If that is not the case then I think we can hit a similar bug. We do: >> >> __uio_register_device -> device_register -> device's refcount goes to >> zero so we do -> uio_device_release -> kfree(idev) >> >> and if it is possible the interrupt handler could still run after >> free_irq then we would end up doing: >> >> uio_interrupt -> mutex_lock(&idev->info_lock) -> idev access freed >> memory. > > I think this shouldn't happen. Because the free_irq function does not > return until any executing interrupts for this IRQ have completed. >
If free_irq returns after executing interrupts and does not allow new executions what is the lock protecting in uio_interrupt?
| |