Re: [vfio-users] missing VFIO_IOMMU_NOTIFY_DMA_UNMAP event when driver hasn't called vfio_pin_pages

Re: [vfio-users] missing VFIO_IOMMU_NOTIFY_DMA_UNMAP event when driver hasn't called vfio_pin_pages

[Alex Williamson]

Probably better to ask on the vfio devel list (kvm) rather than the
vfio user's list...

On Fri, 28 Feb 2020 17:20:20 +0000
Thanos Makatos <thanos.makatos@nutanix.com> wrote:

> > Drivers that handle DMA region registration events without having to call
> > vfio_pin_pages (e.g. in muser we inject the fd backing that VMA to a
> > userspace
> > process and then ask it to memory map that fd) aren't notified at all when
> > that
> > region is unmapped.  Because of this, we get duplicate/overlapping DMA
> > regions
> > that can be problematic to properly handle (e.g. we can implicitly unmap the
> > existing region etc.). Would it make sense for VFIO to always send the DMA
> > unregistration event to a driver (or at least conditionally, if the driver
> > requests it with some flag during driver registration time), even if it doesn't
> > currently have any pages pinned? I think this could be beneficial for drivers
> > other than muser, e.g. some driver set up some bookkeeping data structure
> > in
> > response to the DMA_MAP event but it didn't happen to have to pin any
> > page. By
> > receiving the DMA_UNMAP event it could release that data
> > structure/memory.
> > 
> > I've experimented with a proof of concept which seems to work:
> > 
> > # git diff --cached
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c
> > index d864277ea16f..2aaa88f64c67 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -875,6 +875,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu
> > *iommu,
> >         struct vfio_dma *dma, *dma_last = NULL;
> >         size_t unmapped = 0;
> >         int ret = 0, retries = 0;
> > +       bool advised = false;
> > 
> >         mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> > 
> > @@ -944,9 +945,11 @@ static int vfio_dma_do_unmap(struct vfio_iommu
> > *iommu,
> >                 if (dma->task->mm != current->mm)
> >                         break;
> > 
> > -               if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
> > +               if (!RB_EMPTY_ROOT(&dma->pfn_list) || !advised) {
> >                         struct vfio_iommu_type1_dma_unmap nb_unmap;
> > 
> > +                       advised = true;
> > +

The while() loop iterates over every overlapping mapping, but this
would only advise on the first overlap.  I think instead of this
advised bool logic you'd only unlock the iommu->lock and take the
again: goto if there were pinned pages.

> >                         if (dma_last == dma) {
> >                                 BUG_ON(++retries > 10);
> >                         } else {  
> 
> I have also experimented with sending two overlapping DMA regions to VFIO and
> vfio_dma_do_map explicitly fails this operation with -EEXIST. Therefore I could
> assume that if my driver receives two overlapping DMA regions then the existing
> one can be safely unmapped. However, there is still a possibility of resource
> leak since there is no guarantee that at least part of an unmapped DMA region
> will be clobbered by another one. This could be partially mitigated by
> introducing some timeout to unmap the fd of a DMA region that hasn't been
> accessed for some time (and then mmap it on demand if necessary), but it's still
> not ideal.

Seems like the approach you'd take only after exhausting all options to
get an unmap notification, which we certainly haven't yet.
 
> I still think giving the option to mdev drivers to request to be notified
> for DMA unmap events is the best way to solve this problem. Are there other
> alternatives?

I don't have an immediate problem with using the existing notifier
regardless of whether pages are pinned.  If we had to we could use
another event bit to select all unmaps versus only unmaps overlapping
pinned pages.  Thanks,

Alex