| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Yo5scR3gW5XvEnif@itl-email>
Date: Wed, 25 May 2022 13:50:37 -0400
From: Demi Marie Obenour <demi@...isiblethingslab.com>
To: Juergen Gross <jgross@...e.com>,
Boris Ostrovsky <boris.ostrovsky@...cle.com>,
Stefano Stabellini <sstabellini@...nel.org>,
Jennifer Herbert <jennifer.herbert@...rix.com>,
David Vrabel <david.vrabel@...rix.com>
Cc: xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xen/gntdev: Avoid blocking in unmap_grant_pages()
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@...r.kernel.org
> > Signed-off-by: Demi Marie Obenour <demi@...isiblethingslab.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
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists