On Tue, 13 Dec 2022 07:46:55 -0800 Steve Sistare <steven.sistare@xxxxxxxxxx> 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@xxxxxxxxxx> > --- > 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 > > /*