Re: [RFC PATCH 3/6] selftests/vfio: add sPAPR TCE v2 DMA window helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] `vfio_iommu_map()` incorrectly returns a clobbered `errno` if `VFIO_IOMMU_MAP_DMA` fails.
- [Medium] If memory unregistration fails during unmap, the region is left in the `dma_regions` list despite being successfully unmapped in the IOMMU.
- [Medium] Failed DDW creation leaves `iommu->active_window` pointing to an invalidated window, leading to stale IOVA ranges being returned.
--

commit 2deeaa397a1d09614f068ff5ae82a0732c573cd4
Author: Narayana Murty N <nnmlinux@linux.ibm.com>

selftests/vfio: add sPAPR TCE v2 DMA window helpers

This commit adds helper support for sPAPR TCE v2 DMA windows in the VFIO
selftest library. It tracks the platform default DMA window separately from
selftest-created dynamic DMA windows, and adds helpers to create, remove,
and register memory around DMA map/unmap operations.

> diff --git a/tools/testing/selftests/vfio/lib/iommu.c b/tools/testing/selftests/vfio/lib/iommu.c
> index e86457a715bf7..19d19bb45a14b 100644
> --- a/tools/testing/selftests/vfio/lib/iommu.c
> +++ b/tools/testing/selftests/vfio/lib/iommu.c

[ ... ]

> +	/*
> +	 * Neither default nor DDW is sufficient.
> +	 * Remove only the selftest-created DDW, then create a new DDW.
> +	 */
> +	ret = spapr_tce_remove_window(iommu, &iommu->ddw_window);
> +	if (ret)
> +		return ret;
> +
> +	ret = spapr_tce_create_ddw(iommu, min_size, page_shift);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If spapr_tce_create_ddw() fails here, does it leave iommu->active_window
pointing to a window that was just invalidated by spapr_tce_remove_window()?

If iommu->active_window was previously set to &iommu->ddw_window, it is not
reset to NULL on this error path. Later, spapr_tce_iova_ranges() reads
iommu->active_window->start without checking if the window is valid:

static struct iommu_iova_range *spapr_tce_iova_ranges(...)
{
        ...
        ranges[0].start = iommu->active_window->start;
        ranges[0].last = iommu->active_window->start +
                         iommu->active_window->size - 1;
        ...
}

Could this return stale IOVA ranges to the caller?

> +
> +	iommu->active_window = &iommu->ddw_window;
> +	return 0;
> +}

[ ... ]

> @@ -113,9 +342,27 @@ static int vfio_iommu_map(struct iommu *iommu, struct dma_region *region)
>  		.iova = region->iova,
>  		.size = region->size,
>  	};
> +	int ret;
>  
> -	if (ioctl(iommu->container_fd, VFIO_IOMMU_MAP_DMA, &args))
> +#ifdef __powerpc__
> +	if (iommu_is_spapr_tce_v2(iommu)) {
> +		if (!spapr_tce_iova_inside_window(iommu, region))
> +			return -EINVAL;
> +
> +		ret = spapr_register_memory(iommu, region);
> +		if (ret)
> +			return ret;
> +	}
> +#endif
> +
> +	ret = ioctl(iommu->container_fd, VFIO_IOMMU_MAP_DMA, &args);
> +	if (ret) {
> +#ifdef __powerpc__
> +		if (iommu_is_spapr_tce_v2(iommu))
> +			spapr_unregister_memory(iommu, region);
> +#endif
>  		return -errno;
> +	}

[Severity: Medium]
Can the cleanup call to spapr_unregister_memory() clobber the errno from the
failed VFIO_IOMMU_MAP_DMA ioctl?

Since spapr_unregister_memory() performs its own ioctl, it might overwrite
the thread-local errno. Will this result in vfio_iommu_map() returning the
result of the cleanup operation instead of the actual mapping failure?

>  
>  	return 0;
>  }

[ ... ]

> @@ -177,8 +424,18 @@ static int __vfio_iommu_unmap(int fd, u64 iova, u64 size, u32 flags, u64 *unmapp
>  static int vfio_iommu_unmap(struct iommu *iommu, struct dma_region *region,
>  			    u64 *unmapped)
>  {
> -	return __vfio_iommu_unmap(iommu->container_fd, region->iova,
> -				  region->size, 0, unmapped);
> +	int ret;
> +
> +	ret = __vfio_iommu_unmap(iommu->container_fd, region->iova,
> +				 region->size, 0, unmapped);
> +	if (ret)
> +		return ret;
> +#ifdef __powerpc__
> +	if (iommu_is_spapr_tce_v2(iommu))
> +		ret = spapr_unregister_memory(iommu, region);
> +#endif
> +
> +	return ret;
>  }

[Severity: Medium]
If __vfio_iommu_unmap() succeeds but spapr_unregister_memory() fails, does
returning this error bypass the region cleanup in __iommu_unmap()?

In __iommu_unmap():

    if (iommu->iommufd)
        ret = iommufd_unmap(iommu, region, unmapped);
    else
        ret = vfio_iommu_unmap(iommu, region, unmapped);

    if (ret)
        return ret;

    list_del_init(&region->link);

If we return the unregistration failure here, the early return in
__iommu_unmap() skips list_del_init(). Will this leave the successfully
unmapped region in the dma_regions list and corrupt internal state tracking?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703032806.40946-1-nnmlinux@linux.ibm.com?part=3