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