Re: [PATCH V0 3/3] x86/hyperv: Implement root VM IOMMU kernel only driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iommu: Flawed partial unmap logic causes memory leak and over-unmap in hv_iommu_map_pages
- [High] iommu: Incorrect assignment of IOVA aperture_end restricts DMA address space
--

commit c33bf0d2ad36d04023ab67f38b30621e50f3b924
Author: Mukesh R <mrathor@linux.microsoft.com>

x86/hyperv: Implement root VM IOMMU kernel only driver

Adds a new kernel-only virtual IOMMU driver that works with the Microsoft
Hyper-V hypervisor on privileged root VMs. The driver communicates with the
hypervisor for managing paging domains and device mappings.

> diff --git a/drivers/iommu/hyperv/hv-iommu-root.c b/drivers/iommu/hyperv/hv-iommu-root.c
> new file mode 100644
> index 0000000000000..a5268e0e52cc2
> --- /dev/null
> +++ b/drivers/iommu/hyperv/hv-iommu-root.c
> @@ -0,0 +1,645 @@
[ ... ]
> +static struct iommu_domain *hv_iommu_domain_alloc_paging(struct device *dev)
> +{
[ ... ]
> +	hvdom->domid_num = unique_id;
> +
> +	hvdom->iommu_dom.pgsize_bitmap = HV_IOMMU_PGSIZES;
> +	hvdom->iommu_dom.geometry.aperture_start = 0;
> +	hvdom->iommu_dom.geometry.aperture_end = hv_max_iova_width;

[Severity: High]
Does this directly assign the bit-width value to the maximum address boundary
instead of creating an address mask?

The hypervisor capability max_iova_width returns the bit-width of the
supported IOVA space (e.g., 39 or 48 bits), not the maximum memory address
mask. Since the IOMMU core uses aperture_end as the absolute maximum address
for IOVA allocations, this seems to restrict the DMA address space to a
scalar bit-width value, which could cause any DMA mapping requested above
address 39 or 48 to fail.

> +	hvdom->iommu_dom.geometry.force_aperture = true;
> +	hvdom->iommu_dom.ops = &hv_paging_domain_ops;
[ ... ]
> +static int hv_iommu_map_pages(struct iommu_domain *immdom, ulong iova,
> +			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
> +			      int prot, gfp_t gfp, size_t *mapped)
> +{
[ ... ]
> +	if (!hv_result_success(status)) {
> +		size_t done_size = done << HV_HYP_PAGE_SHIFT;
> +
> +		hv_status_err(status, "pgs:%lx/%lx iova:%lx\n",
> +			      done, npages, iova);
> +		/*
> +		 * lookup tree has all mappings [0 - size-1]. Below unmap will
> +		 * only remove from [0 - done], we need to remove second chunk
> +		 * [done+1 - size-1].
> +		 */
> +		hv_iommu_del_tree_mappings(hvdom, iova, size - done_size);
> +		hv_iommu_unmap_pages(immdom, iova - done_size, HV_HYP_PAGE_SIZE,
> +				     done, NULL);

[Severity: High]
If a batched mapping operation fails midway, does this error path
inadvertently leak mappings and over-unmap pages?

When splitting the software tree mapping, the error path calls
hv_iommu_del_tree_mappings(), which blocks removing the unmapped tail because
it does not support splitting:

drivers/iommu/hyperv/hv-iommu-root.c:hv_iommu_del_tree_mappings() {
    ...
    /* Splitting of a mapping is not supported at present */
    if (mapping->iova.start < iova)
        break;
    ...
}

Subsequently, does the head unmap via hv_iommu_unmap_pages() delete the
entire software node and use its full original size to unmap pages from the
hypervisor?

This seems like it would send out-of-bounds unmap requests for pages never
successfully mapped, and abandon the correctly mapped pages since the
software node is destroyed.

> +		if (mapped)
> +			*mapped = 0;
> +	} else
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921225028.4007330-1-mrathor@linux.microsoft.com?part=3