* Re: [PATCH v1] iommu/virtio: Reset device before deleting virtqueues on probe failure
2026-09-11 1:12 [PATCH v1] iommu/virtio: Reset device before deleting virtqueues on probe failure Yuho Choi
@ 2026-09-11 1:30 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-09-11 1:30 UTC (permalink / raw)
To: Yuho Choi; +Cc: Michael S. Tsirkin, virtualization, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] The driver ignores the return value of `iommu_device_register()`, which can lead to a double `list_del()` and kernel panic upon device removal if registration failed.
- [High] The driver context `vdev->priv` is assigned too late in the probe sequence, creating a race window where an early interrupt causes a NULL pointer dereference.
- [High] DMA mappings for event queue buffers are leaked on the probe error path and during device removal.
- [High] `viommu_fill_evtq()` fails to kick the event queue after adding initial buffers, potentially stalling event reception.
--
commit f64d3ec8a50d9c00ad5c23ca6661968d147a4ff4
Author: Yuho Choi <oss.patchbox@gmail.com>
iommu/virtio: Reset device before deleting virtqueues on probe failure
viommu_probe() marks the device DRIVER_OK before populating the event
virtqueue and registering the IOMMU device in sysfs. If either operation
fails, the error path deletes the virtqueues while the device is still
live. The device may therefore continue accessing queue memory after it
has been freed.
Reset the device on error paths after DRIVER_OK before deleting the
virtqueues, matching viommu_remove().
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 587fc13197f12..fa72ae23b8afa 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -1227,12 +1227,12 @@ static int viommu_probe(struct virtio_device *vdev)
> /* Populate the event queue with buffers */
> ret = viommu_fill_evtq(viommu);
[Severity: High]
This is a pre-existing issue, but viommu_fill_evtq() populates the event
virtqueue with initial buffers via virtqueue_add_inbuf() but never issues a
virtqueue_kick() to notify the host.
According to the Virtio specification, adding buffers to a virtqueue after
the device is marked ready requires an explicit notification.
Can this result in a queue stall, preventing the host from reporting IOMMU
faults?
> if (ret)
> - goto err_free_vqs;
> + goto err_reset_vdev;
>
> ret = iommu_device_sysfs_add(&viommu->iommu, dev, NULL, "%s",
> virtio_bus_name(vdev));
> if (ret)
> - goto err_free_vqs;
> + goto err_reset_vdev;
>
> vdev->priv = viommu;
[Severity: High]
This is a pre-existing issue, but is vdev->priv assigned too late?
Before this assignment, viommu_fill_evtq() populates the event queue with
buffers, and virtio_device_ready() was already called.
If the device immediately consumes these buffers and asserts an interrupt,
viommu_event_handler() will read vq->vdev->priv as NULL and dereference it:
drivers/iommu/virtio-iommu.c:viommu_event_handler() {
...
struct viommu_dev *viommu = vq->vdev->priv;
...
}
Can this lead to a NULL pointer dereference if an early interrupt occurs?
[Severity: High]
This is also a pre-existing issue, but shortly after vdev->priv is assigned,
the return value of iommu_device_register() is ignored:
vdev->priv = viommu;
iommu_device_register(&viommu->iommu, &viommu_ops, parent_dev);
If iommu_device_register() fails, it cleans up the list node by calling
iommu_device_unregister(), poisoning the node. Since the failure is ignored,
viommu_probe() falsely succeeds.
Later, upon device unbind, viommu_remove() unconditionally calls
iommu_device_unregister() again.
Does this cause a double list_del() and a kernel panic?
>
> @@ -1244,6 +1244,8 @@ static int viommu_probe(struct virtio_device *vdev)
>
> return 0;
>
> +err_reset_vdev:
> + virtio_reset_device(vdev);
> err_free_vqs:
> vdev->config->del_vqs(vdev);
[Severity: High]
This isn't a bug introduced by this patch, but virtqueues are deleted here
without first detaching and unmapping the unused buffers.
Since viommu_fill_evtq() allocated and mapped buffers using
virtqueue_add_inbuf(), and del_vqs() doesn't automatically unmap DMA
addresses, are these mappings permanently leaked?
Does the same leak also occur during normal device removal in
viommu_remove()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911011249.1498825-1-oss.patchbox@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread