Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] iommu/virtio: memory leak of pending requests on device removal --
commit 64ef340ce80cdcdb5ed14c36f584818573bc5be9 Author: Anlai Lu <[email protected]> iommu/virtio: stop queueing and draining once the device is removed This commit introduces a removed flag to the viommu_dev struct to indicate when the device is being torn down. It sets the flag under the request_lock in viommu_remove() and checks it in the queueing and syncing paths to prevent queueing or draining operations from running on virtqueues that are being destroyed. > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c > index 1a43664587514..ba1d78e4daa0b 100644 > --- a/drivers/iommu/virtio-iommu.c > +++ b/drivers/iommu/virtio-iommu.c [ ... ] > @@ -206,6 +221,10 @@ static int viommu_sync_req(struct viommu_dev *viommu) > unsigned long flags; > > spin_lock_irqsave(&viommu->request_lock, flags); > + if (!viommu_device_live(viommu)) { > + spin_unlock_irqrestore(&viommu->request_lock, flags); > + return 0; > + } [Severity: Medium] If a device is removed while IOMMU requests are queued but not yet synced, does this early return bypass __viommu_sync_req() where those pending requests would normally be freed? > ret = __viommu_sync_req(viommu); > if (ret) > dev_dbg(viommu->dev, "could not sync requests (%d)\n", ret); [ ... ] > @@ -1555,10 +1577,20 @@ static int viommu_probe(struct virtio_device *vdev) > static void viommu_remove(struct virtio_device *vdev) > { > struct viommu_dev *viommu = vdev->priv; > + unsigned long flags; > > iommu_device_sysfs_remove(&viommu->iommu); > iommu_device_unregister(&viommu->iommu); > > + /* > + * The queues go away here: nothing may be queued or drained from now > + * on. Taking request_lock is what tells a drain in flight that it has > + * to finish before the teardown, since the drain holds it throughout. > + */ > + spin_lock_irqsave(&viommu->request_lock, flags); > + viommu->removed = true; > + spin_unlock_irqrestore(&viommu->request_lock, flags); > + [Severity: Medium] When setting viommu->removed to true during teardown, do we also need to iterate over the viommu->requests list and free any abandoned viommu_request objects? These objects are dynamically allocated here: drivers/iommu/virtio-iommu.c:__viommu_add_req() { ... req = kzalloc_flex(*req, buf, len, GFP_ATOMIC); ... } and bypassing __viommu_sync_req() leaves them without a cleanup path before the queues are destroyed. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
