Re: [PATCH] xen/gntdev: Avoid blocking in unmap_grant_pages()

[Demi Marie Obenour <demi@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 8653 bytes --]

On Wed, May 25, 2022 at 11:17:39AM +0200, Juergen Gross wrote:
> On 24.05.22 18:45, Demi Marie Obenour wrote:
> > unmap_grant_pages() currently waits for the pages to no longer be used.
> > In https://github.com/QubesOS/qubes-issues/issues/7481, this lead to a
> > deadlock against i915: i915 was waiting for gntdev's MMU notifier to
> > finish, while gntdev was waiting for i915 to free its pages.  I also
> > believe this is responsible for various deadlocks I have experienced in
> > the past.
> > 
> > Avoid these problems by making unmap_grant_pages async.  This requires
> > making it return void, as any errors will not be available when the
> > function returns.  Fortunately, the only use of the return value is a
> > WARN_ON().  Replace this with WARN_ON()s where errors are detected.
> 
> Not all callers of unmap_grant_pages() are issuing a WARN_ON(). Are you
> sure that this change can't result in a flood of WARN()s?

It probably can, but for a different reason.  Since the actual unmap is
now asynchronous, the handle is set to INVALID_GRANT_HANDLE too late to
prevent double unmaps.  I will fix this by using a separate bool array
in v2.  Another option is to (ab)use the paddding bits in the hypercall
struct, but that seems too fragile.

> Please note that you are modifying the semantics in case of an unmap
> operation returning an error. Previously there were no further unmaps
> done in this case, while now you are basically continue unmapping even
> after hitting an error. This seems to be fine, but is worth mentioning
> in the commit message.

Will fix in v2, thanks.

> > Fixes: 745282256c75 ("xen/gntdev: safely unmap grants in case they are still in use")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > ---
> >   drivers/xen/gntdev-common.h |  4 ++
> >   drivers/xen/gntdev.c        | 82 ++++++++++++++++++-------------------
> >   2 files changed, 45 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
> > index 20d7d059dadb..a6e2805ea2ce 100644
> > --- a/drivers/xen/gntdev-common.h
> > +++ b/drivers/xen/gntdev-common.h
> > @@ -16,6 +16,7 @@
> >   #include <linux/mmu_notifier.h>
> >   #include <linux/types.h>
> >   #include <xen/interface/event_channel.h>
> > +#include <xen/grant_table.h>
> >   struct gntdev_dmabuf_priv;
> > @@ -73,6 +74,9 @@ struct gntdev_grant_map {
> >   	/* Needed to avoid allocation in gnttab_dma_free_pages(). */
> >   	xen_pfn_t *frames;
> >   #endif
> > +
> > +	/* Needed to avoid allocation in __unmap_grant_pages */
> > +	struct gntab_unmap_queue_data unmap_data;
> >   };
> >   struct gntdev_grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count,
> > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > index 59ffea800079..670d800e4a89 100644
> > --- a/drivers/xen/gntdev.c
> > +++ b/drivers/xen/gntdev.c
> > @@ -62,8 +62,8 @@ MODULE_PARM_DESC(limit,
> >   static int use_ptemod;
> > -static int unmap_grant_pages(struct gntdev_grant_map *map,
> > -			     int offset, int pages);
> > +static void unmap_grant_pages(struct gntdev_grant_map *map,
> > +			      int offset, int pages);
> >   static struct miscdevice gntdev_miscdev;
> > @@ -349,61 +349,65 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map)
> >   	return err;
> >   }
> > -static int __unmap_grant_pages(struct gntdev_grant_map *map, int offset,
> > -			       int pages)
> > +static void __unmap_grant_pages_done(int result,
> > +		struct gntab_unmap_queue_data *data)
> >   {
> > -	int i, err = 0;
> > -	struct gntab_unmap_queue_data unmap_data;
> > -
> > -	if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
> > -		int pgno = (map->notify.addr >> PAGE_SHIFT);
> > -		if (pgno >= offset && pgno < offset + pages) {
> > -			/* No need for kmap, pages are in lowmem */
> > -			uint8_t *tmp = pfn_to_kaddr(page_to_pfn(map->pages[pgno]));
> > -			tmp[map->notify.addr & (PAGE_SIZE-1)] = 0;
> > -			map->notify.flags &= ~UNMAP_NOTIFY_CLEAR_BYTE;
> > -		}
> > -	}
> > -
> > -	unmap_data.unmap_ops = map->unmap_ops + offset;
> > -	unmap_data.kunmap_ops = use_ptemod ? map->kunmap_ops + offset : NULL;
> > -	unmap_data.pages = map->pages + offset;
> > -	unmap_data.count = pages;
> > -
> > -	err = gnttab_unmap_refs_sync(&unmap_data);
> > -	if (err)
> > -		return err;
> > +	int i;
> 
> Mind using unsigned int instead?

Not at all.  I used int because that is what the original code used, but
an unsigned type is definitely better.

> > +	struct gntdev_grant_map *map = data->data;
> > +	int offset = data->unmap_ops - map->unmap_ops;
> > -	for (i = 0; i < pages; i++) {
> > -		if (map->unmap_ops[offset+i].status)
> > -			err = -EINVAL;
> > +	for (i = 0; i < data->count; i++) {
> > +		WARN_ON(map->unmap_ops[offset+i].status); >   		pr_debug("unmap handle=%d st=%d\n",
> >   			map->unmap_ops[offset+i].handle,
> >   			map->unmap_ops[offset+i].status);
> >   		map->unmap_ops[offset+i].handle = INVALID_GRANT_HANDLE;
> >   		if (use_ptemod) {
> > -			if (map->kunmap_ops[offset+i].status)
> > -				err = -EINVAL;
> > +			WARN_ON(map->kunmap_ops[offset+i].status);
> >   			pr_debug("kunmap handle=%u st=%d\n",
> >   				 map->kunmap_ops[offset+i].handle,
> >   				 map->kunmap_ops[offset+i].status);
> >   			map->kunmap_ops[offset+i].handle = INVALID_GRANT_HANDLE;
> >   		}
> >   	}
> > -	return err;
> >   }
> > -static int unmap_grant_pages(struct gntdev_grant_map *map, int offset,
> > -			     int pages)
> > +static void __unmap_grant_pages(struct gntdev_grant_map *map, int offset,
> > +			       int pages)
> > +{
> > +	if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
> > +		int pgno = (map->notify.addr >> PAGE_SHIFT);
> > +
> > +		if (pgno >= offset && pgno < offset + pages) {
> > +			/* No need for kmap, pages are in lowmem */
> > +			uint8_t *tmp = pfn_to_kaddr(page_to_pfn(map->pages[pgno]));
> > +
> > +			tmp[map->notify.addr & (PAGE_SIZE-1)] = 0;
> > +			map->notify.flags &= ~UNMAP_NOTIFY_CLEAR_BYTE;
> > +		}
> > +	}
> > +
> > +	map->unmap_data.unmap_ops = map->unmap_ops + offset;
> > +	map->unmap_data.kunmap_ops = use_ptemod ? map->kunmap_ops + offset : NULL;
> > +	map->unmap_data.pages = map->pages + offset;
> > +	map->unmap_data.count = pages;
> > +	map->unmap_data.done = __unmap_grant_pages_done;
> > +	map->unmap_data.data = map;
> 
> I think you need to call refcount_inc(&map->users) here and do the related
> gntdev_put_map() call in __unmap_grant_pages_done(). Otherwise you are risking
> to have map freed before the async operation has finished.

Whoops, good catch.  Will fix in v2.

> > +
> > +	gnttab_unmap_refs_async(&map->unmap_data);
> > +}
> > +
> > +static void unmap_grant_pages(struct gntdev_grant_map *map, int offset,
> > +			      int pages)
> >   {
> > -	int range, err = 0;
> > +	int range;
> >   	pr_debug("unmap %d+%d [%d+%d]\n", map->index, map->count, offset, pages);
> >   	/* It is possible the requested range will have a "hole" where we
> >   	 * already unmapped some of the grants. Only unmap valid ranges.
> >   	 */
> > -	while (pages && !err) {
> > +	while (pages) {
> >   		while (pages &&
> >   		       map->unmap_ops[offset].handle == INVALID_GRANT_HANDLE) {
> >   			offset++;
> > @@ -416,12 +420,10 @@ static int unmap_grant_pages(struct gntdev_grant_map *map, int offset,
> >   				break;
> >   			range++;
> >   		}
> > -		err = __unmap_grant_pages(map, offset, range);
> > +		__unmap_grant_pages(map, offset, range);
> >   		offset += range;
> >   		pages -= range;
> >   	}
> > -
> > -	return err;
> >   }
> >   /* ------------------------------------------------------------------ */
> > @@ -473,7 +475,6 @@ static bool gntdev_invalidate(struct mmu_interval_notifier *mn,
> >   	struct gntdev_grant_map *map =
> >   		container_of(mn, struct gntdev_grant_map, notifier);
> >   	unsigned long mstart, mend;
> > -	int err;
> >   	if (!mmu_notifier_range_blockable(range))
> >   		return false;
> > @@ -494,10 +495,9 @@ static bool gntdev_invalidate(struct mmu_interval_notifier *mn,
> >   			map->index, map->count,
> >   			map->vma->vm_start, map->vma->vm_end,
> >   			range->start, range->end, mstart, mend);
> > -	err = unmap_grant_pages(map,
> > +	unmap_grant_pages(map,
> >   				(mstart - map->vma->vm_start) >> PAGE_SHIFT,
> >   				(mend - mstart) >> PAGE_SHIFT);
> > -	WARN_ON(err);
> >   	return true;
> >   }
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]