Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()

[Marek Marczykowski-GÃrecki]

On Tue, Aug 25, 2015 at 03:18:17PM +0200, Jiri Slaby wrote:
> On 08/25/2015, 01:52 PM, Marek Marczykowski-GÃrecki wrote:
> >>> --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@
> >>> -529,12 +529,14 @@ static int gntdev_release(struct inode
> >>> *inode, struct file *flip)
> >>> 
> >>> pr_debug("priv %p\n", priv);
> >>> 
> >>> +	mutex_lock(&priv->lock);
> >> 
> >> Since 3.12 doesn't seem to include 1401c00e59ea ("xen/gntdev:
> >> convert priv->lock to a mutex"), this shouldn't be applied as
> >> priv->lock is actually a spinlock.  So, you'll need to pick
> >> 1401c00e59ea or backport this patch using the appropriate locking
> >> directives.  Not sure what's the best solution.  Maybe Marek or
> >> David can help...?
> > 
> > I've used spinlock approach for some time (on 3.18.x) and it works
> > ok. This applies also to 3.10 and 3.14 of course.
> > 
> > Patch here: 
> > https://raw.githubusercontent.com/QubesOS/qubes-linux-kernel/stable-3.
> 18/patches.xen/0001-xen-grant-fix-race-condition-in-gntdev_release.patch
> >
> >  and here:
> > 
> > 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 MIME-Version: 1.0 Content-Type: text/plain;
> > charset=UTF-8 Content-Transfer-Encoding: 8bit Organization:
> > Invisible Things Lab Cc: Marek Marczykowski-GÃrecki
> > <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > 
> > While gntdev_release is called, MMU notifier is still registered
> > and will 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);
> > 
> > +	spin_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)); +
> > spin_unlock(&priv->lock);
> 
> Hmm, but e.g.
> gntdev_put_map
>  -> gntdev_free_map
>    -> free_xenballooned_pages
>      -> mutex_lock
> 
> means sleep inside atomic, right?

Indeed, you're probably right. But I haven't hit that problem ever...

-- 
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: pgpO1nVCjd5Lm.pgp
Description: PGP signature