Re: [PATCH 6/6] iommu/virtio: batch the mapping replay on domain attach

[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 queued requests on device removal
--

commit db185ffd41ba01e7309f7ee2d8c108a5d39c6450
Author: Anlai Lu <agicy@qq.com>

iommu/virtio: batch the mapping replay on domain attach

This commit optimizes the domain attach process by queueing all mapping
requests and waiting for them once, rather than sending them synchronously
one by one. This significantly reduces the IRQ-off window and attach time
when dealing with a large number of mappings.

> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 4e578d4255f6b..7b883f59221f5 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
[ ... ]
> @@ -656,23 +698,57 @@ static int viommu_domain_map_identity(struct viommu_endpoint *vdev,
>  }
>  
>  /*
> - * viommu_replay_mappings - re-send MAP requests
> + * viommu_replay_mappings - send every mapping of the domain again
[ ... ]
> +	while (!done) {
> +		struct viommu_mapping *mapping;
> +		struct virtio_iommu_req_map map;
[ ... ]
> +		ret = viommu_add_req(vdomain->viommu, &map, sizeof(map), &status);
>  
> -		ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
> -		if (ret) {
[ ... ]
> +		if (mapping->iova.last == ULONG_MAX)
> +			done = true;
> +		else
> +			next_iova = mapping->iova.last + 1;
> +		spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
>  
> -		node = interval_tree_iter_next(node, 0, -1UL);
> +		if (ret)
> +			break;
>  	}
> -	spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
>  
> -	return ret;
> +	/* Wait for all of them outside the lock: this is the batched drain */
> +	viommu_sync_req(vdomain->viommu);

[Severity: Medium]
If the virtio-iommu device is removed (e.g., via hot-unplug or driver unbind)
while we are queuing these batched requests, could this leak the dynamically
allocated struct viommu_request objects?

Looking at viommu_sync_req(), it short-circuits when the device is removed
without draining the list:

drivers/iommu/virtio-iommu.c:viommu_sync_req() {
	...
	if (!viommu_device_live(viommu)) {
		spin_unlock_irqrestore(&viommu->request_lock, flags);
		return 0;
	}
	...
}

Any queued requests would remain in viommu->requests. Since viommu_remove()
tears down the virtqueues and sets viommu->removed = true, but does not
iterate over the viommu->requests list to free the pending elements, it
appears these requests would permanently leak.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_85AC6E749F6A912B7BB9FF360F8ECB3F5C09@qq.com?part=6