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 <[email protected]> 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/[email protected]?part=1
