linux-kernel - Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 25 Aug 2015 15:18:17 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	Marek Marczykowski-Górecki 
	<marmarek@...isiblethingslab.com>,
	Luis Henriques <luis.henriques@...onical.com>
Cc:	stable@...r.kernel.org, linux-kernel@...r.kernel.org,
	David Vrabel <david.vrabel@...rix.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in
 gntdev_release()

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

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@...isiblethingslab.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
> <marmarek@...isiblethingslab.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
> <marmarek@...isiblethingslab.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?

thanks,
- -- 
js
suse labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV3GsVAAoJEL0lsQQGtHBJyfUP/Ro99Km/OWSpK3SRKzsVDo9z
OwYzvnHdHNR8uqcS/VnzWfZokine+qWFVNdqfWx3GPRL03wt1JUdjdZTA+cIi2Xn
P6PcxUXTz3aCz7vPV79nW0yLpaG+aUoCeNGFvVTfJQnz/4gNQ0DaVkD2wmy6GptX
ce1mW4AofkQl5lFaWOb5xehy/qGzaj9egtZKfVrGcrAj8oRSHCOCdgW6qMbztBkI
YRlZY2pBgGmkx3o6PktqouKx4zi9akEK1j9/axDzhjQxc2Put36P4XuqQIfgVqQr
H2EZZD5JF8HhgIWlOvwo7Ll0TEmpaQ8ouxBUhHtNTFjYtR+r8hTwl6PfZLH9qWzT
IkOF7gDMHkbm73dLG3r+pSOyLsX9f0koYFFLHyRNKPEuYjRYV65hzRgSj1feXuni
l5VG2vO6YFJcmjPh8q0pKciIWvNw+4n10XEwub1Qk/PVjGZDAaTKIWw2H4q/ZAqk
9+0zQzlzGC78pGQsdLmnjFchZ7ye9/F8zuDPhNEv4MgjoI2gq1zfGgWb+bZeq4JX
3/TLh33U/a9Zlbb1HCjs/bN3l+WlYCwOqVgSwUyZwem8gZrPlJ37aukCbolHecUj
xk07IaY1Wv4OHYzxdXeSd9XeUospM19WSYoVaTVBi7RBrKTHdBYHGeQxocYBgviG
Ckj9O9Pe+NfT4qC0Rj6I
=6tTn
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/