Messages in this thread | | | Subject | Re: [PATCH v3 3/3] uio: fix crash after the device is unregistered | From | Xiubo Li <> | Date | Tue, 10 Jul 2018 10:40:17 +0800 |
| |
On 2018/7/10 1:06, Mike Christie wrote: > 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? > I meant idev->info->handler(irq, idev->info), it may should be protected by the related lock in each driver.
Thanks,
| |