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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hyperv: map_pages causes sleep in atomic context by ignoring gfp flag
- [High] hyperv: memory leak and unmap failures due to unsafe interval tree iteration
- [Medium] hyperv: undefined behavior via 64-bit shift in hv_iommu_init()
--

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

    x86/hyperv: Implement root VM IOMMU kernel only driver
    
    Add a new file to implement a kernel only virtual IOMMU that works
    with Microsoft Hyper-V hypervisor (aka MSHV) on privileged VMs aka
    root VMs. The hypervisor claims the IOMMU upon boot, and this driver
    communicates with it for creating and deleting paging domains, attaching
    of devices, mapping and unmapping of pages, etc. During boot, hypervisor
    automatically creates identity and blocked domains, so there is no need
    to do hypercalls to create them.  This is a kernel only driver and only
    supported on baremetal root (and not L1VH root) without any guest passthru
    support. Support for guest device passthru will be added incrementally.

> diff --git a/drivers/iommu/hyperv/hv-iommu-root.c b/drivers/iommu/hyperv/hv-iommu-root.c
> new file mode 100644
> index 0000000000000..4c6ebaa8fb971
> --- /dev/null
> +++ b/drivers/iommu/hyperv/hv-iommu-root.c
> @@ -0,0 +1,656 @@

[ ... ]

> +static size_t hv_iommu_del_tree_mappings(struct hv_domain *hvdom, ulong iova,
> +					 size_t size)
> +{
> +	ulong flags;
> +	size_t unmapped = 0;
> +	ulong last = iova + size - 1;
> +	struct hv_iommu_mapping *mapping = NULL;
> +	struct interval_tree_node *node, *next;
> +
> +	spin_lock_irqsave(&hvdom->mappings_lock, flags);
> +	next = interval_tree_iter_first(&hvdom->mappings_tree, iova, last);
> +	while (next) {
> +		node = next;
> +		mapping = container_of(node, struct hv_iommu_mapping, iova);
> +		next = interval_tree_iter_next(node, iova, last);
> +
> +		/* Splitting of a mapping is not supported at present */
> +		if (mapping->iova.start < iova)
> +			break;
> +
> +		unmapped += mapping->iova.last - mapping->iova.start + 1;
> +
> +		interval_tree_remove(node, &hvdom->mappings_tree);
> +		kfree(mapping);

[Severity: High]
Does this code skip overlapping mappings and leak memory?

This loop removes nodes from an interval tree using interval_tree_remove() while
iterating over it using interval_tree_iter_next(). Since interval_tree_remove()
alters the tree's structure (rebalances it and updates __subtree_last), invoking
interval_tree_iter_next() after the tree has been rebalanced can invalidate the
traversal path.

This can cause the traversal to skip remaining overlapping nodes, leaking memory
and preventing the hypervisor from unmapping those IOVA pages.

> +	}
> +	spin_unlock_irqrestore(&hvdom->mappings_lock, flags);
> +
> +	return unmapped;
> +}

[ ... ]

> +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)
> +{

[ ... ]

> +	npages = size >> HV_HYP_PAGE_SHIFT;
> +	while (done < npages) {
> +		ulong completed, remain = npages - done;
> +
> +		remain = min(remain, HV_MAP_DEVICE_GPA_BATCH_SIZE);
> +
> +		status = hv_iommu_map_pgs(hvdom, iova, paddr, remain,
> +					  map_flags);
> +
> +		completed = hv_repcomp(status);
> +		done = done + completed;
> +		iova = iova + (completed << HV_HYP_PAGE_SHIFT);
> +		paddr = paddr + (completed << HV_HYP_PAGE_SHIFT);
> +
> +		if (hv_result_needs_memory(status)) {
> +			ret = hv_call_deposit_pages(NUMA_NO_NODE,
> +						    hv_current_partition_id,
> +						    256);

[Severity: High]
Can this sequence cause a "scheduling while atomic" panic?

IOMMU map operations are routinely called from atomic or interrupt contexts,
such as DMA mapping via dma_map_single().

The caller-provided gfp flag is ignored here. The code calls
hv_call_deposit_pages(), which unconditionally allocates memory using GFP_KERNEL
(which can sleep) to satisfy hypervisor page table demands.

> +			if (ret)
> +				break;
> +			continue;
> +		}
> +		if (!hv_result_success(status))
> +			break;
> +	}

[ ... ]

> +static int __init hv_iommu_init(void)
> +{
> +	int rc;
> +	struct iommu_device *iommup = &hv_virt_iommu;
> +	struct hv_output_get_iommu_capabilities caps;
> +
> +	if (!hv_is_hyperv_initialized())
> +		return -ENODEV;
> +
> +	rc = hv_iommu_get_caps(&caps);
> +	if (rc)
> +		return rc;
> +
> +	hv_iommu_max_iova = ((ulong)1 << caps.max_iova_width) - 1;

[Severity: Medium]
Will this evaluate to 0 if max_iova_width is 64?

If the hypervisor supports 64-bit addressing and returns caps.max_iova_width as
64, this line will compute (1UL << 64) - 1. On x86-64 architectures, shift amounts
are masked modulo 64 (i.e. 64 & 63 = 0), so 1UL << 0 = 1, resulting in 1 - 1 = 0.

This undefined behavior could corrupt the IOMMU geometry's aperture_end and
break the driver for hardware devices using high IOVAs.

> +
> +	rc = iommu_device_sysfs_add(iommup, NULL, NULL, "%s", "hyperv-iommu");
> +	if (rc) {

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