On Tue, 13 Dec 2022 11:54:33 -0500
Steven Sistare <steven.sistare@xxxxxxxxxx> wrote:
> On 12/13/2022 11:26 AM, Alex Williamson wrote:
> > 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,
>
> Will do. I think I will put the revert at the end, though, as dead code
> clean up. That patch will be larger, and if it is judged to be too large
> for stable, it can be omitted from stable.
>
> - Steve
>
> >> +
> >> /* 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;
> >> +}
I'd also keep this generic to what it's actually doing, ie. simply
reporting if emulated_iommu_groups are present, so it could be
something like:
static bool vfio_iommu_has_emulated(struct vfio_iommu *iommu)
OTOH, I'm not sure it actually makes sense to dynamically change
reported value, the IOMMU backend supports vaddr update, but there are
usage restrictions and there's no way that this test can't be racy w/
other user actions. Does it have specific utility in userspace to test
for this immediately before a live-update, or would QEMU enable support
for the feature based only on some initial condition? Thanks,
Alex
> >> +
> >> 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
> >>
> >> /*
> >
>
