linux-kernel - Re: [PATCH v5 4/5] vfio: selftests: update DMA map/unmap helpers to support more test kinds

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aQACJucKne4DRv06@google.com>
Date: Mon, 27 Oct 2025 23:37:10 +0000
From: David Matlack <dmatlack@...gle.com>
To: Alex Mastro <amastro@...com>
Cc: Alex Williamson <alex@...zbot.org>, Jason Gunthorpe <jgg@...pe.ca>,
	Alejandro Jimenez <alejandro.j.jimenez@...cle.com>,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 4/5] vfio: selftests: update DMA map/unmap helpers to
 support more test kinds

On 2025-10-27 10:33 AM, Alex Mastro wrote:
> Add __vfio_pci_dma_* helpers which return -errno from the underlying
> ioctls.
> 
> Add __vfio_pci_dma_unmap_all to test more unmapping code paths. Add an
> out unmapped arg to report the unmapped byte size.

nit: Please append () to function names in commit messages and comments
(e.g. "Add __vfio_pci_dma_unmap_all() to test ..."). It helps make it
obvious you are referring to a function.

> The existing vfio_pci_dma_* functions, which are intended for happy-path
> usage (assert on failure) are now thin wrappers on top of the
> double-underscore helpers.
> 
> Signed-off-by: Alex Mastro <amastro@...com>

Aside from the commit message and the braces nits,

  Reviewed-by: David Matlack <dmatlack@...gle.com>

> @@ -152,10 +153,13 @@ static void vfio_iommu_dma_map(struct vfio_pci_device *device,
>  		.size = region->size,
>  	};
>  
> -	ioctl_assert(device->container_fd, VFIO_IOMMU_MAP_DMA, &args);
> +	if (ioctl(device->container_fd, VFIO_IOMMU_MAP_DMA, &args))
> +		return -errno;

Interesting. I was imagining this would would return whatever ioctl()
returned and then the caller could check errno if it wanted to. But I
actually like this better, since it simplifies the assertions at the
caller (like in your next patch).

> +int __vfio_pci_dma_unmap_all(struct vfio_pci_device *device, u64 *unmapped)
> +{
> +	int ret;
> +	struct vfio_dma_region *curr, *next;
> +
> +	if (device->iommufd)
> +		ret = iommufd_dma_unmap(device->iommufd, 0, UINT64_MAX,
> +					device->ioas_id, unmapped);

This reminds me, I need to get rid of INVALID_IOVA in vfio_util.h.

__to_iova() can just return int for success/error and pass the iova up
to the caller via parameter.

> +	else
> +		ret = vfio_iommu_dma_unmap(device->container_fd, 0, 0,
> +					   VFIO_DMA_UNMAP_FLAG_ALL, unmapped);
> +
> +	if (ret)
> +		return ret;
> +
> +	list_for_each_entry_safe(curr, next, &device->dma_regions, link) {
> +		list_del_init(&curr->link);
> +	}

nit: No need for {} for single-line loop.