Re: [Xen-devel] race condition in xen-gntdev

[Marek Marczykowski-GÃrecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Jun 22, 2015 at 03:14:16PM -0400, Daniel De Graaf wrote:
> The reason that gntdev_release didn't have a lock is because there are not
> supposed to be any references to the areas pointed to by priv->maps when it
> is called.  However, since the MMU notifier has not yet been unregistered,
> it is apparently possible to race here; the comment on mmu_notifier_unregister
> seems to confirm this as a possibility (as do the backtraces).
> 
> I think adding the lock will be sufficient.

Ok, so here is the patch:

-----------8<------------

From b876e14888bdafa112c3265e6420543fa74aa709 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
Date: Fri, 26 Jun 2015 02:16:49 +0200
Subject: [PATCH] xen/grant: fix race condition in gntdev_release

While gntdev_release is called, MMU notifier is still registered and
can traverse priv->maps list even if no pages are mapped (which is the
case - gntdev_release is called after all). But gntdev_release will
clear that list, so make sure that only one of those things happens at
the same time.

Signed-off-by: Marek Marczykowski-GÃrecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
---
 drivers/xen/gntdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 8927485..4bd23bb 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -568,12 +568,14 @@ static int gntdev_release(struct inode *inode, struct 
file *flip)
 
        pr_debug("priv %p\n", priv);
 
+       mutex_lock(&priv->lock);
        while (!list_empty(&priv->maps)) {
                map = list_entry(priv->maps.next, struct grant_map, next);
                list_del(&map->next);
                gntdev_put_map(NULL /* already removed */, map);
        }
        WARN_ON(!list_empty(&priv->freeable_maps));
+       mutex_unlock(&priv->lock);
 
        if (use_ptemod)
                mmu_notifier_unregister(&priv->mn, priv->mm);
-- 
1.9.3


-- 
Best Regards,
Marek Marczykowski-GÃrecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: pgpo20zT_oTeH.pgp
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel