Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 08/25/2015, 04:08 PM, Marek Marczykowski-Górecki wrote: >> 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... Ok, so I dropped 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 from 3.12. thanks, - -- js suse labs -BEGIN PGP SIGNATURE- Version: GnuPG v2 iQIcBAEBCAAGBQJV3sN2AAoJEL0lsQQGtHBJkIAP/Regy1kPHkB8dlHejk9IF+pO 8AG2VfeeDq9XTX94mb02AApgZbD+/0yLegCKoidaEpgjUNxpxcocrYo0K7dJQnAM ThYsuhaXVp1H13nOt7a3IpXtz1290qpYe+t6es9uA5Dp/HW7Stv4vI82lN1B9S1l FphFXt1GiHEr6hSF6YkQgRo9b0//hmY/pYbakkB7gwxUmelvBpNoOdqukTpOSKZ9 3asF2cpxQ2kHY3EoFTM16PbUlyH91FF8j/F/AChYQfOaM7ikChIfMKz0bXIbR/FH esmP5IDkEdA01M1irtUjjzdtBSAOBM/9Ii2YhxAE4rPk/MV0fi5Lg8G4iBDLF4Fu +/80G2UsYmR17AD/LIZ/ohSaO3Z9tXTZrJjPrqPAetZcieXBSast5r5LldN4WiHG DilbDNU6zVgk2OHG1cJwVR8xvXTohwXveY4vrEPLLZWyyBYEh1ABBf0/NBgQvbKY JqooOFU3Z1mPNRBko+IbIM9VaZtx9WLgR4QUwsoZVS8ItHIqZGnHxkEhMn8arjl1 K++xL/v2YK2iwCyyqR83tTBsFEg3DFFI4M0MTAh+LyZkW5/weHlVDwlCV6qdkOvW JMJgdivgrHl2K1CGYIjXW8aOtKYq1GSI398JQPfFOlsX37wNiBrB3xODq2Eph7nS 3k7uCgwbeyj3coICr8Tm =OuQx -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 08/25/2015, 04:08 PM, Marek Marczykowski-Górecki wrote: 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... Ok, so I dropped 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 from 3.12. thanks, - -- js suse labs -BEGIN PGP SIGNATURE- Version: GnuPG v2 iQIcBAEBCAAGBQJV3sN2AAoJEL0lsQQGtHBJkIAP/Regy1kPHkB8dlHejk9IF+pO 8AG2VfeeDq9XTX94mb02AApgZbD+/0yLegCKoidaEpgjUNxpxcocrYo0K7dJQnAM ThYsuhaXVp1H13nOt7a3IpXtz1290qpYe+t6es9uA5Dp/HW7Stv4vI82lN1B9S1l FphFXt1GiHEr6hSF6YkQgRo9b0//hmY/pYbakkB7gwxUmelvBpNoOdqukTpOSKZ9 3asF2cpxQ2kHY3EoFTM16PbUlyH91FF8j/F/AChYQfOaM7ikChIfMKz0bXIbR/FH esmP5IDkEdA01M1irtUjjzdtBSAOBM/9Ii2YhxAE4rPk/MV0fi5Lg8G4iBDLF4Fu +/80G2UsYmR17AD/LIZ/ohSaO3Z9tXTZrJjPrqPAetZcieXBSast5r5LldN4WiHG DilbDNU6zVgk2OHG1cJwVR8xvXTohwXveY4vrEPLLZWyyBYEh1ABBf0/NBgQvbKY JqooOFU3Z1mPNRBko+IbIM9VaZtx9WLgR4QUwsoZVS8ItHIqZGnHxkEhMn8arjl1 K++xL/v2YK2iwCyyqR83tTBsFEg3DFFI4M0MTAh+LyZkW5/weHlVDwlCV6qdkOvW JMJgdivgrHl2K1CGYIjXW8aOtKYq1GSI398JQPfFOlsX37wNiBrB3xODq2Eph7nS 3k7uCgwbeyj3coICr8Tm =OuQx -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
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(>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?=
> > 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
> >
> >
> > 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
> > --- 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(>lock); while (!list_empty(>maps)) { map =
> > list_entry(priv->maps.next, struct grant_map, next);
> > list_del(>next); gntdev_put_map(NULL /* already removed */,
> > map); } WARN_ON(!list_empty(>freeable_maps)); +
> > spin_unlock(>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
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(>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?=
> 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
>
>
> 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
> --- 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(>lock); while (!list_empty(>maps)) { map =
> list_entry(priv->maps.next, struct grant_map, next);
> list_del(>next); gntdev_put_map(NULL /* already removed */,
> map); } WARN_ON(!list_empty(>freeable_maps)); +
> spin_unlock(>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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
On Tue, Aug 25, 2015 at 12:35:59PM +0100, Luis Henriques wrote:
> [ Adding Greg has he seems to have this patch queued for 3.10 and 3.14 ]
>
> On Mon, Aug 24, 2015 at 11:09:09AM +0200, Jiri Slaby wrote:
> > From: Marek Marczykowski-Górecki
> >
> > 3.12-stable review patch. If anyone has any objections, please let me know.
> >
> > ===
> >
> > commit 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 upstream.
> >
> > While gntdev_release() is called the 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
> > Signed-off-by: David Vrabel
> > Signed-off-by: Jiri Slaby
> > ---
> > drivers/xen/gntdev.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > index e41c79c986ea..f2ca8d0af55f 100644
> > --- 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(>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?=
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
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
---
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(>lock);
while (!list_empty(>maps)) {
map = list_entry(priv->maps.next, struct grant_map, next);
list_del(>next);
gntdev_put_map(NULL /* already removed */, map);
}
WARN_ON(!list_empty(>freeable_maps));
+ spin_unlock(>lock);
if (use_ptemod)
mmu_notifier_unregister(>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?
pgpo5DpdaXWTv.pgp
Description: PGP signature
Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
[ Adding Greg has he seems to have this patch queued for 3.10 and 3.14 ]
On Mon, Aug 24, 2015 at 11:09:09AM +0200, Jiri Slaby wrote:
> From: Marek Marczykowski-Górecki
>
> 3.12-stable review patch. If anyone has any objections, please let me know.
>
> ===
>
> commit 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 upstream.
>
> While gntdev_release() is called the 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
> Signed-off-by: David Vrabel
> Signed-off-by: Jiri Slaby
> ---
> drivers/xen/gntdev.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index e41c79c986ea..f2ca8d0af55f 100644
> --- 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(>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...?
Cheers,
--
Luís
> while (!list_empty(>maps)) {
> map = list_entry(priv->maps.next, struct grant_map, next);
> list_del(>next);
> gntdev_put_map(NULL /* already removed */, map);
> }
> WARN_ON(!list_empty(>freeable_maps));
> + mutex_unlock(>lock);
>
> if (use_ptemod)
> mmu_notifier_unregister(>mn, priv->mm);
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
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
Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
[ Adding Greg has he seems to have this patch queued for 3.10 and 3.14 ]
On Mon, Aug 24, 2015 at 11:09:09AM +0200, Jiri Slaby wrote:
From: Marek Marczykowski-Górecki marma...@invisiblethingslab.com
3.12-stable review patch. If anyone has any objections, please let me know.
===
commit 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 upstream.
While gntdev_release() is called the 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 marma...@invisiblethingslab.com
Signed-off-by: David Vrabel david.vra...@citrix.com
Signed-off-by: Jiri Slaby jsl...@suse.cz
---
drivers/xen/gntdev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index e41c79c986ea..f2ca8d0af55f 100644
--- 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...?
Cheers,
--
Luís
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);
--
2.5.0
--
To unsubscribe from this list: send the line unsubscribe stable in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
On Tue, Aug 25, 2015 at 12:35:59PM +0100, Luis Henriques wrote:
[ Adding Greg has he seems to have this patch queued for 3.10 and 3.14 ]
On Mon, Aug 24, 2015 at 11:09:09AM +0200, Jiri Slaby wrote:
From: Marek Marczykowski-Górecki marma...@invisiblethingslab.com
3.12-stable review patch. If anyone has any objections, please let me know.
===
commit 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 upstream.
While gntdev_release() is called the 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 marma...@invisiblethingslab.com
Signed-off-by: David Vrabel david.vra...@citrix.com
Signed-off-by: Jiri Slaby jsl...@suse.cz
---
drivers/xen/gntdev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index e41c79c986ea..f2ca8d0af55f 100644
--- 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);
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?
pgpo5DpdaXWTv.pgp
Description: PGP signature
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?=
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?
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
From: Marek Marczykowski-Górecki
3.12-stable review patch. If anyone has any objections, please let me know.
===
commit 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 upstream.
While gntdev_release() is called the 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
Signed-off-by: David Vrabel
Signed-off-by: Jiri Slaby
---
drivers/xen/gntdev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index e41c79c986ea..f2ca8d0af55f 100644
--- 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(>lock);
while (!list_empty(>maps)) {
map = list_entry(priv->maps.next, struct grant_map, next);
list_del(>next);
gntdev_put_map(NULL /* already removed */, map);
}
WARN_ON(!list_empty(>freeable_maps));
+ mutex_unlock(>lock);
if (use_ptemod)
mmu_notifier_unregister(>mn, priv->mm);
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev_release()
From: Marek Marczykowski-Górecki marma...@invisiblethingslab.com
3.12-stable review patch. If anyone has any objections, please let me know.
===
commit 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 upstream.
While gntdev_release() is called the 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 marma...@invisiblethingslab.com
Signed-off-by: David Vrabel david.vra...@citrix.com
Signed-off-by: Jiri Slaby jsl...@suse.cz
---
drivers/xen/gntdev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index e41c79c986ea..f2ca8d0af55f 100644
--- 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);
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);
--
2.5.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
