Re: [Xen-devel] [PATCH v2 09/15] xen/gntdev: use mmu_range_notifier_insert

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 10/28/19 4:10 PM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
>
> gntdev simply wants to monitor a specific VMA for any notifier events,
> this can be done straightforwardly using mmu_range_notifier_insert() over
> the VMA's VA range.
>
> The notifier should be attached until the original VMA is destroyed.
>
> It is unclear if any of this is even sane, but at least a lot of duplicate
> code is removed.

I didn't have a chance to look at the patch itself yet but as a heads-up
--- it crashes dom0.

-boris


>
> Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> ---
>  drivers/xen/gntdev-common.h |   8 +-
>  drivers/xen/gntdev.c        | 180 ++++++++++--------------------------
>  2 files changed, 49 insertions(+), 139 deletions(-)
>
> diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
> index 2f8b949c3eeb14..b201fdd20b667b 100644
> --- a/drivers/xen/gntdev-common.h
> +++ b/drivers/xen/gntdev-common.h
> @@ -21,15 +21,8 @@ struct gntdev_dmabuf_priv;
>  struct gntdev_priv {
>       /* Maps with visible offsets in the file descriptor. */
>       struct list_head maps;
> -     /*
> -      * Maps that are not visible; will be freed on munmap.
> -      * Only populated if populate_freeable_maps == 1
> -      */
> -     struct list_head freeable_maps;
>       /* lock protects maps and freeable_maps. */
>       struct mutex lock;
> -     struct mm_struct *mm;
> -     struct mmu_notifier mn;
>  
>  #ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>       /* Device for which DMA memory is allocated. */
> @@ -49,6 +42,7 @@ struct gntdev_unmap_notify {
>  };
>  
>  struct gntdev_grant_map {
> +     struct mmu_range_notifier notifier;
>       struct list_head next;
>       struct vm_area_struct *vma;
>       int index;
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index a446a7221e13e9..12d626670bebbc 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -65,7 +65,6 @@ MODULE_PARM_DESC(limit, "Maximum number of grants that may 
> be mapped by "
>  static atomic_t pages_mapped = ATOMIC_INIT(0);
>  
>  static int use_ptemod;
> -#define populate_freeable_maps use_ptemod
>  
>  static int unmap_grant_pages(struct gntdev_grant_map *map,
>                            int offset, int pages);
> @@ -251,12 +250,6 @@ void gntdev_put_map(struct gntdev_priv *priv, struct 
> gntdev_grant_map *map)
>               evtchn_put(map->notify.event);
>       }
>  
> -     if (populate_freeable_maps && priv) {
> -             mutex_lock(&priv->lock);
> -             list_del(&map->next);
> -             mutex_unlock(&priv->lock);
> -     }
> -
>       if (map->pages && !use_ptemod)
>               unmap_grant_pages(map, 0, map->count);
>       gntdev_free_map(map);
> @@ -445,17 +438,9 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
>       struct gntdev_priv *priv = file->private_data;
>  
>       pr_debug("gntdev_vma_close %p\n", vma);
> -     if (use_ptemod) {
> -             /* It is possible that an mmu notifier could be running
> -              * concurrently, so take priv->lock to ensure that the vma won't
> -              * vanishing during the unmap_grant_pages call, since we will
> -              * spin here until that completes. Such a concurrent call will
> -              * not do any unmapping, since that has been done prior to
> -              * closing the vma, but it may still iterate the unmap_ops list.
> -              */
> -             mutex_lock(&priv->lock);
> +     if (use_ptemod && map->vma == vma) {
> +             mmu_range_notifier_remove(&map->notifier);
>               map->vma = NULL;
> -             mutex_unlock(&priv->lock);
>       }
>       vma->vm_private_data = NULL;
>       gntdev_put_map(priv, map);
> @@ -477,109 +462,44 @@ static const struct vm_operations_struct gntdev_vmops 
> = {
>  
>  /* ------------------------------------------------------------------ */
>  
> -static bool in_range(struct gntdev_grant_map *map,
> -                           unsigned long start, unsigned long end)
> -{
> -     if (!map->vma)
> -             return false;
> -     if (map->vma->vm_start >= end)
> -             return false;
> -     if (map->vma->vm_end <= start)
> -             return false;
> -
> -     return true;
> -}
> -
> -static int unmap_if_in_range(struct gntdev_grant_map *map,
> -                           unsigned long start, unsigned long end,
> -                           bool blockable)
> +static bool gntdev_invalidate(struct mmu_range_notifier *mn,
> +                           const struct mmu_notifier_range *range,
> +                           unsigned long cur_seq)
>  {
> +     struct gntdev_grant_map *map =
> +             container_of(mn, struct gntdev_grant_map, notifier);
>       unsigned long mstart, mend;
>       int err;
>  
> -     if (!in_range(map, start, end))
> -             return 0;
> +     if (!mmu_notifier_range_blockable(range))
> +             return false;
>  
> -     if (!blockable)
> -             return -EAGAIN;
> +     /*
> +      * If the VMA is split or otherwise changed the notifier is not
> +      * updated, but we don't want to process VA's outside the modified
> +      * VMA. FIXME: It would be much more understandable to just prevent
> +      * modifying the VMA in the first place.
> +      */
> +     if (map->vma->vm_start >= range->end ||
> +         map->vma->vm_end <= range->start)
> +             return true;
>  
> -     mstart = max(start, map->vma->vm_start);
> -     mend   = min(end,   map->vma->vm_end);
> +     mstart = max(range->start, map->vma->vm_start);
> +     mend = min(range->end, map->vma->vm_end);
>       pr_debug("map %d+%d (%lx %lx), range %lx %lx, mrange %lx %lx\n",
>                       map->index, map->count,
>                       map->vma->vm_start, map->vma->vm_end,
> -                     start, end, mstart, mend);
> +                     range->start, range->end, mstart, mend);
>       err = unmap_grant_pages(map,
>                               (mstart - map->vma->vm_start) >> PAGE_SHIFT,
>                               (mend - mstart) >> PAGE_SHIFT);
>       WARN_ON(err);
>  
> -     return 0;
> -}
> -
> -static int mn_invl_range_start(struct mmu_notifier *mn,
> -                            const struct mmu_notifier_range *range)
> -{
> -     struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
> -     struct gntdev_grant_map *map;
> -     int ret = 0;
> -
> -     if (mmu_notifier_range_blockable(range))
> -             mutex_lock(&priv->lock);
> -     else if (!mutex_trylock(&priv->lock))
> -             return -EAGAIN;
> -
> -     list_for_each_entry(map, &priv->maps, next) {
> -             ret = unmap_if_in_range(map, range->start, range->end,
> -                                     mmu_notifier_range_blockable(range));
> -             if (ret)
> -                     goto out_unlock;
> -     }
> -     list_for_each_entry(map, &priv->freeable_maps, next) {
> -             ret = unmap_if_in_range(map, range->start, range->end,
> -                                     mmu_notifier_range_blockable(range));
> -             if (ret)
> -                     goto out_unlock;
> -     }
> -
> -out_unlock:
> -     mutex_unlock(&priv->lock);
> -
> -     return ret;
> -}
> -
> -static void mn_release(struct mmu_notifier *mn,
> -                    struct mm_struct *mm)
> -{
> -     struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
> -     struct gntdev_grant_map *map;
> -     int err;
> -
> -     mutex_lock(&priv->lock);
> -     list_for_each_entry(map, &priv->maps, next) {
> -             if (!map->vma)
> -                     continue;
> -             pr_debug("map %d+%d (%lx %lx)\n",
> -                             map->index, map->count,
> -                             map->vma->vm_start, map->vma->vm_end);
> -             err = unmap_grant_pages(map, /* offset */ 0, map->count);
> -             WARN_ON(err);
> -     }
> -     list_for_each_entry(map, &priv->freeable_maps, next) {
> -             if (!map->vma)
> -                     continue;
> -             pr_debug("map %d+%d (%lx %lx)\n",
> -                             map->index, map->count,
> -                             map->vma->vm_start, map->vma->vm_end);
> -             err = unmap_grant_pages(map, /* offset */ 0, map->count);
> -             WARN_ON(err);
> -     }
> -     mutex_unlock(&priv->lock);
> +     return true;
>  }
>  
> -static const struct mmu_notifier_ops gntdev_mmu_ops = {
> -     .release                = mn_release,
> -     .invalidate_range_start = mn_invl_range_start,
> +static const struct mmu_range_notifier_ops gntdev_mmu_ops = {
> +     .invalidate = gntdev_invalidate,
>  };
>  
>  /* ------------------------------------------------------------------ */
> @@ -594,7 +514,6 @@ static int gntdev_open(struct inode *inode, struct file 
> *flip)
>               return -ENOMEM;
>  
>       INIT_LIST_HEAD(&priv->maps);
> -     INIT_LIST_HEAD(&priv->freeable_maps);
>       mutex_init(&priv->lock);
>  
>  #ifdef CONFIG_XEN_GNTDEV_DMABUF
> @@ -606,17 +525,6 @@ static int gntdev_open(struct inode *inode, struct file 
> *flip)
>       }
>  #endif
>  
> -     if (use_ptemod) {
> -             priv->mm = get_task_mm(current);
> -             if (!priv->mm) {
> -                     kfree(priv);
> -                     return -ENOMEM;
> -             }
> -             priv->mn.ops = &gntdev_mmu_ops;
> -             ret = mmu_notifier_register(&priv->mn, priv->mm);
> -             mmput(priv->mm);
> -     }
> -
>       if (ret) {
>               kfree(priv);
>               return ret;
> @@ -653,16 +561,12 @@ static int gntdev_release(struct inode *inode, struct 
> file *flip)
>               list_del(&map->next);
>               gntdev_put_map(NULL /* already removed */, map);
>       }
> -     WARN_ON(!list_empty(&priv->freeable_maps));
>       mutex_unlock(&priv->lock);
>  
>  #ifdef CONFIG_XEN_GNTDEV_DMABUF
>       gntdev_dmabuf_fini(priv->dmabuf_priv);
>  #endif
>  
> -     if (use_ptemod)
> -             mmu_notifier_unregister(&priv->mn, priv->mm);
> -
>       kfree(priv);
>       return 0;
>  }
> @@ -723,8 +627,6 @@ static long gntdev_ioctl_unmap_grant_ref(struct 
> gntdev_priv *priv,
>       map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count);
>       if (map) {
>               list_del(&map->next);
> -             if (populate_freeable_maps)
> -                     list_add_tail(&map->next, &priv->freeable_maps);
>               err = 0;
>       }
>       mutex_unlock(&priv->lock);
> @@ -1096,11 +998,6 @@ static int gntdev_mmap(struct file *flip, struct 
> vm_area_struct *vma)
>               goto unlock_out;
>       if (use_ptemod && map->vma)
>               goto unlock_out;
> -     if (use_ptemod && priv->mm != vma->vm_mm) {
> -             pr_warn("Huh? Other mm?\n");
> -             goto unlock_out;
> -     }
> -
>       refcount_inc(&map->users);
>  
>       vma->vm_ops = &gntdev_vmops;
> @@ -1111,10 +1008,6 @@ static int gntdev_mmap(struct file *flip, struct 
> vm_area_struct *vma)
>               vma->vm_flags |= VM_DONTCOPY;
>  
>       vma->vm_private_data = map;
> -
> -     if (use_ptemod)
> -             map->vma = vma;
> -
>       if (map->flags) {
>               if ((vma->vm_flags & VM_WRITE) &&
>                               (map->flags & GNTMAP_readonly))
> @@ -1125,8 +1018,28 @@ static int gntdev_mmap(struct file *flip, struct 
> vm_area_struct *vma)
>                       map->flags |= GNTMAP_readonly;
>       }
>  
> +     if (use_ptemod) {
> +             map->vma = vma;
> +             err = mmu_range_notifier_insert_locked(
> +                     &map->notifier, vma->vm_start,
> +                     vma->vm_end - vma->vm_start, vma->vm_mm);
> +             if (err)
> +                     goto out_unlock_put;
> +     }
>       mutex_unlock(&priv->lock);
>  
> +     /*
> +      * gntdev takes the address of the PTE in find_grant_ptes() and passes
> +      * it to the hypervisor in gntdev_map_grant_pages(). The purpose of
> +      * the notifier is to prevent the hypervisor pointer to the PTE from
> +      * going stale.
> +      *
> +      * Since this vma's mappings can't be touched without the mmap_sem,
> +      * and we are holding it now, there is no need for the notifier_range
> +      * locking pattern.
> +      */
> +     mmu_range_read_begin(&map->notifier);
> +
>       if (use_ptemod) {
>               map->pages_vm_start = vma->vm_start;
>               err = apply_to_page_range(vma->vm_mm, vma->vm_start,
> @@ -1175,8 +1088,11 @@ static int gntdev_mmap(struct file *flip, struct 
> vm_area_struct *vma)
>       mutex_unlock(&priv->lock);
>  out_put_map:
>       if (use_ptemod) {
> -             map->vma = NULL;
>               unmap_grant_pages(map, 0, map->count);
> +             if (map->vma) {
> +                     mmu_range_notifier_remove(&map->notifier);
> +                     map->vma = NULL;
> +             }
>       }
>       gntdev_put_map(priv, map);
>       return err;


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel