Re: [PATCH 4/6] iommu/virtio: stop queueing and draining once the device is removed

[sashiko-bot@kernel.org]

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 <agicy@qq.com>

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/tencent_85AC6E749F6A912B7BB9FF360F8ECB3F5C09@qq.com?part=4