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

[Thanos Makatos]

> 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;
> +
>                         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.

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?