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?=
> > <marma...@invisiblethingslab.com> 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
> > <marma...@invisiblethingslab.com>
> >
> > 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
> > <marma...@invisiblethingslab.com> --- 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?
pgpM3fxpRmHQI.pgp
Description: PGP signature
