Re: [PATCH V1 1/2] vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 13 Dec 2022 07:46:55 -0800
Steve Sistare <steven.sistare@oracle.com> wrote:

> Disable the VFIO_UPDATE_VADDR capability if mediated devices are present.
> Their kernel threads could be blocked indefinitely by a misbehaving
> userland while trying to pin/unpin pages while vaddrs are being updated.

Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")

> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 25 ++++++++++++++++++++++++-
>  include/uapi/linux/vfio.h       |  6 +++++-
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 23c24fe..f81e925 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1343,6 +1343,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>  
>  	mutex_lock(&iommu->lock);
>  
> +	/* Cannot update vaddr if mdev is present. */
> +	if (invalidate_vaddr && !list_empty(&iommu->emulated_iommu_groups))
> +		goto unlock;
> +
>  	pgshift = __ffs(iommu->pgsize_bitmap);
>  	pgsize = (size_t)1 << pgshift;
>  
> @@ -2189,6 +2193,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  
>  	mutex_lock(&iommu->lock);
>  
> +	/* Prevent an mdev from sneaking in while vaddr flags are used. */
> +	if (iommu->vaddr_invalid_count && type == VFIO_EMULATED_IOMMU)
> +		goto out_unlock;

Why only mdev devices?  If we restrict that the user cannot attach a
group while there are invalid vaddrs, and the pin/unpin pages and
dma_rw interfaces are restricted to cases where vaddr_invalid_count is
zero, then we can get rid of all the code to handle waiting for vaddrs.
ie. we could still revert:

898b9eaeb3fe ("vfio/type1: block on invalid vaddr")
487ace134053 ("vfio/type1: implement notify callback")
ec5e32940cc9 ("vfio: iommu driver notify callback")

It appears to me it might be easiest to lead with a clean revert of
these, then follow-up imposing the usage restrictions, and I'd go ahead
and add WARN_ON error paths to the pin/unpin/dma_rw paths to make sure
nobody enters those paths with an elevated invalid count.  Thanks,

Alex

> +
>  	/* Check for duplicates */
>  	if (vfio_iommu_find_iommu_group(iommu, iommu_group))
>  		goto out_unlock;
> @@ -2660,6 +2668,20 @@ static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
>  	return ret;
>  }
>  
> +/*
> + * Disable this feature if mdevs are present.  They cannot safely pin/unpin
> + * while vaddrs are being updated.
> + */
> +static int vfio_iommu_can_update_vaddr(struct vfio_iommu *iommu)
> +{
> +	int ret;
> +
> +	mutex_lock(&iommu->lock);
> +	ret = list_empty(&iommu->emulated_iommu_groups);
> +	mutex_unlock(&iommu->lock);
> +	return ret;
> +}
> +
>  static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>  					    unsigned long arg)
>  {
> @@ -2668,8 +2690,9 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>  	case VFIO_TYPE1v2_IOMMU:
>  	case VFIO_TYPE1_NESTING_IOMMU:
>  	case VFIO_UNMAP_ALL:
> -	case VFIO_UPDATE_VADDR:
>  		return 1;
> +	case VFIO_UPDATE_VADDR:
> +		return iommu && vfio_iommu_can_update_vaddr(iommu);
>  	case VFIO_DMA_CC_IOMMU:
>  		if (!iommu)
>  			return 0;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index d7d8e09..6d36b84 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -49,7 +49,11 @@
>  /* Supports VFIO_DMA_UNMAP_FLAG_ALL */
>  #define VFIO_UNMAP_ALL			9
>  
> -/* Supports the vaddr flag for DMA map and unmap */
> +/*
> + * Supports the vaddr flag for DMA map and unmap.  Not supported for mediated
> + * devices, so this capability is subject to change as groups are added or
> + * removed.
> + */
>  #define VFIO_UPDATE_VADDR		10
>  
>  /*