Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Ian Campbell]

[resending to correct stable address, sorry folks]

TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
needs $something doing to it, either s/mutex/spinlock/ or (more likely)
backporting of 1401c00e59e too.

Looking at LTS:

3.18.y:	  Backported both.
3.16.y:	  Has backported neither
3.14.y:	* Only backported 30b03d05e074
3.12.y:	  Has backported neither
3.10.y:	* Only backported 30b03d05e074
3.4.y:	  Has backported neither
3.2.y:	  Has backported neither

So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
backporting 1401c00e59e.

3.16/12/4/2 might need to be careful if they subsequently pick up 30b03d05.

See below for the build log error.

On Tue, 2015-09-01 at 11:05 +0100, Ian Campbell wrote:
> On Tue, 2015-09-01 at 10:57 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [linux-3.14 bisection] complete test-amd64
> > -i386
> > -xl-qcow2"):
> > > On Wed, 2015-08-26 at 20:02 +0000, osstest service owner wrote:
> > > >   commit 9e6c072a69d87100808d16279d60e9f857291340
> > > >   Author: Marek Marczykowski-Górecki <
> > > > marmarek@invisiblethingslab.com
> > > > > 
> > > >   Date:   Fri Jun 26 03:28:24 2015 +0200
> > > >   
> > > >       xen/gntdevt: Fix race condition in gntdev_release()
> > > 
> > > I'm not sure what to make of this.
> > > 
> > > The qcow2 test is one of the only ones I'd expect to be exercising 
> > > gntdev
> > > (most tests use LVM+blkback), which explains why this particular 
> > > commit 
> > > is
> > > apparently seeing issues due to this particular change.
> > 
> > (You mean `which explains why this particular _test_ is [failing]',
> > I think.)
> 
> Indeed.
> 
> > The host serial log in one of the confirmation tests of 9e6c072a shows
> > serious trouble:
> > 
> >  http://logs.test-lab.xenproject.org/osstest/logs/60893/test-amd64-i386
> > -xl-qcow2/serial-huxelrebe1.log
> > 
> >  Aug 26 19:36:51.841068 [  738.050547] BUG: unable to handle kernel
> >  NULL pointer dereference at 00000014
> > 
> >  Aug 26 19:36:56.753068 [  738.050594] IP: []
> >  __mmu_notifier_invalidate_range_start+0x33/0x70
> > 
> > And the immediately preceding confirmation flight, which got a pass on
> > 9e6c072a~1, seems fine:
> > 
> >  http://logs.test-lab.xenproject.org/osstest/logs/60892/test-amd64-i386
> > -xl-qcow2/serial-huxelrebe1.log
> > 
> > But, it's difficult to see how that gntdev fix would be responsible
> > for the bug.  Perhaps it changes the order in which certain things
> > happen so as to expose another bug.
> 
> Or perhaps there was a fix and/or change in behaviour in the mmunotifier
> stuff which the patch relied on but which isn't in 3.14?

Looking at http://logs.test-lab.xenproject.org/osstest/logs/60949/'s build
jobs:
http://logs.test-lab.xenproject.org/osstest/logs/60949/build-amd64
-pvops/5.ts-kernel-build.log contains:

drivers/xen/gntdev.c: In function ‘gntdev_release’:
drivers/xen/gntdev.c:532:2: warning: passing argument 1 of ‘mutex_lock’
from incompatible pointer type [enabled by default]
In file included from include/linux/notifier.h:13:0,
                 from include/linux/memory_hotplug.h:6,
                 from include/linux/mmzone.h:821,
                 from include/linux/gfp.h:5,
                 from include/linux/kmod.h:22,
                 from include/linux/module.h:13,
                 from drivers/xen/gntdev.c:24:
include/linux/mutex.h:157:13: note: expected ‘struct mutex *’ but
argument is of type ‘struct spinlock_t *’
drivers/xen/gntdev.c:539:2: warning: passing argument 1 of
‘mutex_unlock’ from incompatible pointer type [enabled by default]
In file included from include/linux/notifier.h:13:0,
                 from include/linux/memory_hotplug.h:6,
                 from include/linux/mmzone.h:821,
                 from include/linux/gfp.h:5,
                 from include/linux/kmod.h:22,
                 from include/linux/module.h:13,
                 from drivers/xen/gntdev.c:24:
include/linux/mutex.h:174:13: note: expected ‘struct mutex *’ but
argument is of type ‘struct spinlock_t *’

Which is somehow a warning and not a build failure.

Ian.

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Luis Henriques]

On Wed, Sep 02, 2015 at 10:18:32AM +0100, Ian Campbell wrote:
> [resending to correct stable address, sorry folks]
> 
> TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
> ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
> needs $something doing to it, either s/mutex/spinlock/ or (more likely)
> backporting of 1401c00e59e too.
> 
> Looking at LTS:
> 
> 3.18.y:	  Backported both.
> 3.16.y:	  Has backported neither
> 3.14.y:	* Only backported 30b03d05e074
> 3.12.y:	  Has backported neither
> 3.10.y:	* Only backported 30b03d05e074
> 3.4.y:	  Has backported neither
> 3.2.y:	  Has backported neither
> 
> So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
> backporting 1401c00e59e.
> 
> 3.16/12/4/2 might need to be careful if they subsequently pick up 30b03d05.
>

Thank you Ian.  In fact, I had explicitly dropped 30b03d05e074
("xen/gntdevt: Fix race condition in gntdev_release()") from the 3.16
kernel and notified stable maintainers about this problem (in a reply to a
3.12 review email).

Simply replacing the mutex by the spinlock in this commit seems to cause
problems (sleep in atomic) as pointed out by Jiri in other thread.

Since 1401c00e59ea ("xen/gntdev: convert priv->lock to a mutex") is a
clean cherry-pick for 3.16 (and probably to older kernels as well), I'm
happy to pick both commits if you can confirm they are both good for older
stable kernels (they seem to be!)

Cheers,
--
Luís

> See below for the build log error.
> 
> On Tue, 2015-09-01 at 11:05 +0100, Ian Campbell wrote:
> > On Tue, 2015-09-01 at 10:57 +0100, Ian Jackson wrote:
> > > Ian Campbell writes ("Re: [linux-3.14 bisection] complete test-amd64
> > > -i386
> > > -xl-qcow2"):
> > > > On Wed, 2015-08-26 at 20:02 +0000, osstest service owner wrote:
> > > > >   commit 9e6c072a69d87100808d16279d60e9f857291340
> > > > >   Author: Marek Marczykowski-Górecki <
> > > > > marmarek@invisiblethingslab.com
> > > > > > 
> > > > >   Date:   Fri Jun 26 03:28:24 2015 +0200
> > > > >   
> > > > >       xen/gntdevt: Fix race condition in gntdev_release()
> > > > 
> > > > I'm not sure what to make of this.
> > > > 
> > > > The qcow2 test is one of the only ones I'd expect to be exercising 
> > > > gntdev
> > > > (most tests use LVM+blkback), which explains why this particular 
> > > > commit 
> > > > is
> > > > apparently seeing issues due to this particular change.
> > > 
> > > (You mean `which explains why this particular _test_ is [failing]',
> > > I think.)
> > 
> > Indeed.
> > 
> > > The host serial log in one of the confirmation tests of 9e6c072a shows
> > > serious trouble:
> > > 
> > >  http://logs.test-lab.xenproject.org/osstest/logs/60893/test-amd64-i386
> > > -xl-qcow2/serial-huxelrebe1.log
> > > 
> > >  Aug 26 19:36:51.841068 [  738.050547] BUG: unable to handle kernel
> > >  NULL pointer dereference at 00000014
> > > 
> > >  Aug 26 19:36:56.753068 [  738.050594] IP: []
> > >  __mmu_notifier_invalidate_range_start+0x33/0x70
> > > 
> > > And the immediately preceding confirmation flight, which got a pass on
> > > 9e6c072a~1, seems fine:
> > > 
> > >  http://logs.test-lab.xenproject.org/osstest/logs/60892/test-amd64-i386
> > > -xl-qcow2/serial-huxelrebe1.log
> > > 
> > > But, it's difficult to see how that gntdev fix would be responsible
> > > for the bug.  Perhaps it changes the order in which certain things
> > > happen so as to expose another bug.
> > 
> > Or perhaps there was a fix and/or change in behaviour in the mmunotifier
> > stuff which the patch relied on but which isn't in 3.14?
> 
> Looking at http://logs.test-lab.xenproject.org/osstest/logs/60949/'s build
> jobs:
> http://logs.test-lab.xenproject.org/osstest/logs/60949/build-amd64
> -pvops/5.ts-kernel-build.log contains:
> 
> drivers/xen/gntdev.c: In function ‘gntdev_release’:
> drivers/xen/gntdev.c:532:2: warning: passing argument 1 of ‘mutex_lock’
> from incompatible pointer type [enabled by default]
> In file included from include/linux/notifier.h:13:0,
>                  from include/linux/memory_hotplug.h:6,
>                  from include/linux/mmzone.h:821,
>                  from include/linux/gfp.h:5,
>                  from include/linux/kmod.h:22,
>                  from include/linux/module.h:13,
>                  from drivers/xen/gntdev.c:24:
> include/linux/mutex.h:157:13: note: expected ‘struct mutex *’ but
> argument is of type ‘struct spinlock_t *’
> drivers/xen/gntdev.c:539:2: warning: passing argument 1 of
> ‘mutex_unlock’ from incompatible pointer type [enabled by default]
> In file included from include/linux/notifier.h:13:0,
>                  from include/linux/memory_hotplug.h:6,
>                  from include/linux/mmzone.h:821,
>                  from include/linux/gfp.h:5,
>                  from include/linux/kmod.h:22,
>                  from include/linux/module.h:13,
>                  from drivers/xen/gntdev.c:24:
> include/linux/mutex.h:174:13: note: expected ‘struct mutex *’ but
> argument is of type ‘struct spinlock_t *’
> 
> Which is somehow a warning and not a build failure.
> 
> Ian.
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[David Vrabel]

On 03/09/15 12:05, Luis Henriques wrote:
> On Wed, Sep 02, 2015 at 10:18:32AM +0100, Ian Campbell wrote:
>> [resending to correct stable address, sorry folks]
>>
>> TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
>> ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
>> needs $something doing to it, either s/mutex/spinlock/ or (more likely)
>> backporting of 1401c00e59e too.
>>
>> Looking at LTS:
>>
>> 3.18.y:	  Backported both.
>> 3.16.y:	  Has backported neither
>> 3.14.y:	* Only backported 30b03d05e074
>> 3.12.y:	  Has backported neither
>> 3.10.y:	* Only backported 30b03d05e074
>> 3.4.y:	  Has backported neither
>> 3.2.y:	  Has backported neither
>>
>> So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
>> backporting 1401c00e59e.
>>
>> 3.16/12/4/2 might need to be careful if they subsequently pick up 30b03d05.
>>
> 
> Thank you Ian.  In fact, I had explicitly dropped 30b03d05e074
> ("xen/gntdevt: Fix race condition in gntdev_release()") from the 3.16
> kernel and notified stable maintainers about this problem (in a reply to a
> 3.12 review email).
> 
> Simply replacing the mutex by the spinlock in this commit seems to cause
> problems (sleep in atomic) as pointed out by Jiri in other thread.
> 
> Since 1401c00e59ea ("xen/gntdev: convert priv->lock to a mutex") is a
> clean cherry-pick for 3.16 (and probably to older kernels as well), I'm
> happy to pick both commits if you can confirm they are both good for older
> stable kernels (they seem to be!)

You can take both 1401c00e59ea and 30b03d05e074.

David

Re: [Xen-devel] [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Luis Henriques]

On Thu, Sep 03, 2015 at 12:16:39PM +0100, David Vrabel wrote:
> On 03/09/15 12:05, Luis Henriques wrote:
> > On Wed, Sep 02, 2015 at 10:18:32AM +0100, Ian Campbell wrote:
> >> [resending to correct stable address, sorry folks]
> >>
> >> TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
> >> ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
> >> needs $something doing to it, either s/mutex/spinlock/ or (more likely)
> >> backporting of 1401c00e59e too.
> >>
> >> Looking at LTS:
> >>
> >> 3.18.y:	  Backported both.
> >> 3.16.y:	  Has backported neither
> >> 3.14.y:	* Only backported 30b03d05e074
> >> 3.12.y:	  Has backported neither
> >> 3.10.y:	* Only backported 30b03d05e074
> >> 3.4.y:	  Has backported neither
> >> 3.2.y:	  Has backported neither
> >>
> >> So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
> >> backporting 1401c00e59e.
> >>
> >> 3.16/12/4/2 might need to be careful if they subsequently pick up 30b03d05.
> >>
> > 
> > Thank you Ian.  In fact, I had explicitly dropped 30b03d05e074
> > ("xen/gntdevt: Fix race condition in gntdev_release()") from the 3.16
> > kernel and notified stable maintainers about this problem (in a reply to a
> > 3.12 review email).
> > 
> > Simply replacing the mutex by the spinlock in this commit seems to cause
> > problems (sleep in atomic) as pointed out by Jiri in other thread.
> > 
> > Since 1401c00e59ea ("xen/gntdev: convert priv->lock to a mutex") is a
> > clean cherry-pick for 3.16 (and probably to older kernels as well), I'm
> > happy to pick both commits if you can confirm they are both good for older
> > stable kernels (they seem to be!)
> 
> You can take both 1401c00e59ea and 30b03d05e074.

Awesome, thanks for confirming.  I'll queue them for the next release of
3.16.

Cheers,
--
Lu�s

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Ian Campbell]

ping for 3.10.y and 3.14.y

Thanks,
Ian.

On Wed, 2015-09-02 at 10:18 +0100, Ian Campbell wrote:
> [resending to correct stable address, sorry folks]
> 
> TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
> ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
> needs $something doing to it, either s/mutex/spinlock/ or (more likely)
> backporting of 1401c00e59e too.
> 
> Looking at LTS:
> 
> 3.18.y:	  Backported both.
> 3.16.y:	  Has backported neither
> 3.14.y:	* Only backported 30b03d05e074
> 3.12.y:	  Has backported neither
> 3.10.y:	* Only backported 30b03d05e074
> 3.4.y:	  Has backported neither
> 3.2.y:	  Has backported neither
> 
> So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
> backporting 1401c00e59e.
> 
> 3.16/12/4/2 might need to be careful if they subsequently pick up
> 30b03d05.
> 
> See below for the build log error.
> 
> On Tue, 2015-09-01 at 11:05 +0100, Ian Campbell wrote:
> > On Tue, 2015-09-01 at 10:57 +0100, Ian Jackson wrote:
> > > Ian Campbell writes ("Re: [linux-3.14 bisection] complete test-amd64
> > > -i386
> > > -xl-qcow2"):
> > > > On Wed, 2015-08-26 at 20:02 +0000, osstest service owner wrote:
> > > > >   commit 9e6c072a69d87100808d16279d60e9f857291340
> > > > >   Author: Marek Marczykowski-Górecki <
> > > > > marmarek@invisiblethingslab.com
> > > > > > 
> > > > >   Date:   Fri Jun 26 03:28:24 2015 +0200
> > > > >   
> > > > >       xen/gntdevt: Fix race condition in gntdev_release()
> > > > 
> > > > I'm not sure what to make of this.
> > > > 
> > > > The qcow2 test is one of the only ones I'd expect to be exercising 
> > > > gntdev
> > > > (most tests use LVM+blkback), which explains why this particular 
> > > > commit 
> > > > is
> > > > apparently seeing issues due to this particular change.
> > > 
> > > (You mean `which explains why this particular _test_ is [failing]',
> > > I think.)
> > 
> > Indeed.
> > 
> > > The host serial log in one of the confirmation tests of 9e6c072a
> > > shows
> > > serious trouble:
> > > 
> > >  http://logs.test-lab.xenproject.org/osstest/logs/60893/test-amd64-i3
> > > 86
> > > -xl-qcow2/serial-huxelrebe1.log
> > > 
> > >  Aug 26 19:36:51.841068 [  738.050547] BUG: unable to handle kernel
> > >  NULL pointer dereference at 00000014
> > > 
> > >  Aug 26 19:36:56.753068 [  738.050594] IP: []
> > >  __mmu_notifier_invalidate_range_start+0x33/0x70
> > > 
> > > And the immediately preceding confirmation flight, which got a pass
> > > on
> > > 9e6c072a~1, seems fine:
> > > 
> > >  http://logs.test-lab.xenproject.org/osstest/logs/60892/test-amd64-i3
> > > 86
> > > -xl-qcow2/serial-huxelrebe1.log
> > > 
> > > But, it's difficult to see how that gntdev fix would be responsible
> > > for the bug.  Perhaps it changes the order in which certain things
> > > happen so as to expose another bug.
> > 
> > Or perhaps there was a fix and/or change in behaviour in the
> > mmunotifier
> > stuff which the patch relied on but which isn't in 3.14?
> 
> Looking at http://logs.test-lab.xenproject.org/osstest/logs/60949/'s buil
> d
> jobs:
> http://logs.test-lab.xenproject.org/osstest/logs/60949/build-amd64
> -pvops/5.ts-kernel-build.log contains:
> 
> drivers/xen/gntdev.c: In function ‘gntdev_release’:
> drivers/xen/gntdev.c:532:2: warning: passing argument 1 of
> ‘mutex_lock’
> from incompatible pointer type [enabled by default]
> In file included from include/linux/notifier.h:13:0,
>                  from include/linux/memory_hotplug.h:6,
>                  from include/linux/mmzone.h:821,
>                  from include/linux/gfp.h:5,
>                  from include/linux/kmod.h:22,
>                  from include/linux/module.h:13,
>                  from drivers/xen/gntdev.c:24:
> include/linux/mutex.h:157:13: note: expected ‘struct mutex *’ but
> argument is of type ‘struct spinlock_t *’
> drivers/xen/gntdev.c:539:2: warning: passing argument 1 of
> ‘mutex_unlock’ from incompatible pointer type [enabled by default]
> In file included from include/linux/notifier.h:13:0,
>                  from include/linux/memory_hotplug.h:6,
>                  from include/linux/mmzone.h:821,
>                  from include/linux/gfp.h:5,
>                  from include/linux/kmod.h:22,
>                  from include/linux/module.h:13,
>                  from drivers/xen/gntdev.c:24:
> include/linux/mutex.h:174:13: note: expected ‘struct mutex *’ but
> argument is of type ‘struct spinlock_t *’
> 
> Which is somehow a warning and not a build failure.
> 
> Ian.

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Greg Kroah-Hartman]

On Fri, Sep 11, 2015 at 04:10:46PM +0100, Ian Campbell wrote:
> ping for 3.10.y and 3.14.y

My stable queue is huge at the moment, and I've been on vacation and am
now just finally catching up with it, don't worry, it's not lost, just
might take a few weeks...

thanks,

greg k-h

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Ian Campbell]

On Fri, 2015-09-11 at 08:51 -0700, Greg Kroah-Hartman wrote:
> On Fri, Sep 11, 2015 at 04:10:46PM +0100, Ian Campbell wrote:
> > ping for 3.10.y and 3.14.y
> 
> My stable queue is huge at the moment, and I've been on vacation and am
> now just finally catching up with it, don't worry, it's not lost, just
> might take a few weeks...

Understood & thanks!

Ian.

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Greg Kroah-Hartman]

On Fri, Sep 11, 2015 at 04:55:00PM +0100, Ian Campbell wrote:
> On Fri, 2015-09-11 at 08:51 -0700, Greg Kroah-Hartman wrote:
> > On Fri, Sep 11, 2015 at 04:10:46PM +0100, Ian Campbell wrote:
> > > ping for 3.10.y and 3.14.y
> > 
> > My stable queue is huge at the moment, and I've been on vacation and am
> > now just finally catching up with it, don't worry, it's not lost, just
> > might take a few weeks...
> 
> Understood & thanks!

Should now be resolved, if not, please let me know.

thanks,

greg k-h

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Ian Campbell]

On Sat, 2015-09-26 at 10:30 -0700, Greg Kroah-Hartman wrote:
> On Fri, Sep 11, 2015 at 04:55:00PM +0100, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 08:51 -0700, Greg Kroah-Hartman wrote:
> > > On Fri, Sep 11, 2015 at 04:10:46PM +0100, Ian Campbell wrote:
> > > > ping for 3.10.y and 3.14.y
> > > 
> > > My stable queue is huge at the moment, and I've been on vacation and
> > > am
> > > now just finally catching up with it, don't worry, it's not lost,
> > > just
> > > might take a few weeks...
> > 
> > Understood & thanks!
> 
> Should now be resolved, if not, please let me know.

Thanks, but I don't see a backport of 1401c00e59ea in either 3.10.89 or
3.14.53.

30b03d05e074 was in 3.10.87 and 3.14.51 and causes breakage in those trees
when 1401c00e59ea is not also present.

Ian.

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Ian Campbell]

On Mon, 2015-09-28 at 10:27 +0100, Ian Campbell wrote:
> On Sat, 2015-09-26 at 10:30 -0700, Greg Kroah-Hartman wrote:
> > On Fri, Sep 11, 2015 at 04:55:00PM +0100, Ian Campbell wrote:
> > > On Fri, 2015-09-11 at 08:51 -0700, Greg Kroah-Hartman wrote:
> > > > On Fri, Sep 11, 2015 at 04:10:46PM +0100, Ian Campbell wrote:
> > > > > ping for 3.10.y and 3.14.y
> > > > 
> > > > My stable queue is huge at the moment, and I've been on vacation
> > > > and
> > > > am
> > > > now just finally catching up with it, don't worry, it's not lost,
> > > > just
> > > > might take a few weeks...
> > > 
> > > Understood & thanks!
> > 
> > Should now be resolved, if not, please let me know.
> 
> Thanks, but I don't see a backport of 1401c00e59ea in either 3.10.89 or
> 3.14.53.

Ah, I was confused into thinking you meant resolved in the releases you did
at the weekend.

I spotted these in stable-queue.git now and will expect them in 3.10.90 and
3.14.54, thanks.

Ian.

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

On Wed, 2015-09-02 at 10:18 +0100, Ian Campbell wrote:
> [resending to correct stable address, sorry folks]
> 
> TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
> ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
> needs $something doing to it, either s/mutex/spinlock/ or (more likely)
> backporting of 1401c00e59e too.
> 
> Looking at LTS:
> 
> 3.18.y:> 	>   Backported both.
> 3.16.y:> 	>   Has backported neither
> 3.14.y:> 	> * Only backported 30b03d05e074
> 3.12.y:> 	>   Has backported neither
> 3.10.y:> 	> * Only backported 30b03d05e074
> 3.4.y:> 	>   Has backported neither
> 3.2.y:> 	>   Has backported neither
> 
> So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
> backporting 1401c00e59e.
> 
> 3.16/12/4/2 might need to be careful if they subsequently pick up 30b03d05.
[...]

I came up with the patch below for 3.2.  Let me know if it's not
correct.

Ben.

---
From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Date: Fri, 26 Jun 2015 03:28:24 +0200
Subject: xen/gntdevt: Fix race condition in gntdev_release()

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 <marmarek@invisiblethingslab.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
[bwh: Backported to 3.2:
 - Adjust context
 - gntdev_priv::lock is a spinlock rather than a mutex.  As gntdev_put_map()
   may sleep, we need to unlock inside the loop.]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -493,11 +493,15 @@ static int gntdev_release(struct inode *
 
 	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);
+		spin_unlock(&priv->lock);
 		gntdev_put_map(map);
+		spin_lock(&priv->lock);
 	}
+	spin_unlock(&priv->lock);
 
 	if (use_ptemod)
 		mmu_notifier_unregister(&priv->mn, priv->mm);

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Ian Campbell]

On Thu, 2015-10-08 at 23:14 +0100, Ben Hutchings wrote:
> On Wed, 2015-09-02 at 10:18 +0100, Ian Campbell wrote:
> > [resending to correct stable address, sorry folks]
> > 
> > TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
> > ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
> > needs $something doing to it, either s/mutex/spinlock/ or (more likely)
> > backporting of 1401c00e59e too.
> > 
> > Looking at LTS:
> > 
> > 3.18.y:> 	>   Backported both.
> > 3.16.y:> 	>   Has backported neither
> > 3.14.y:> 	> * Only backported 30b03d05e074
> > 3.12.y:> 	>   Has backported neither
> > 3.10.y:> 	> * Only backported 30b03d05e074
> > 3.4.y:> 	>   Has backported neither
> > 3.2.y:> 	>   Has backported neither
> > 
> > So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
> > backporting 1401c00e59e.
> > 
> > 3.16/12/4/2 might need to be careful if they subsequently pick up
> > 30b03d05.
> [...]
> 
> I came up with the patch below for 3.2.  Let me know if it's not
> correct.

FWIW most of the other stable branches just took 1401c00e59e which is now
pretty well baked in mainline as well as various stable trees and saves
having to reason about dropping the lock over gntdev_put_map. Was
backporting that one to 3.2 problematic?

I suppose it is safe to drop the lock because map is removed from the list
with the lock held, but I'm not 100% confident in that, and this gntdev
stuff isn't really my bailiwick anyway so I'll back away now...

Ian.

> 
> Ben.
> 
> ---
> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Date: Fri, 26 Jun 2015 03:28:24 +0200
> Subject: xen/gntdevt: Fix race condition in gntdev_release()
> 
> 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 <
> marmarek@invisiblethingslab.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> [bwh: Backported to 3.2:
>  - Adjust context
>  - gntdev_priv::lock is a spinlock rather than a mutex.  As
> gntdev_put_map()
>    may sleep, we need to unlock inside the loop.]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -493,11 +493,15 @@ static int gntdev_release(struct inode *
>  
>  	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);
> +		spin_unlock(&priv->lock);
>  		gntdev_put_map(map);
> +		spin_lock(&priv->lock);
>  	}
> +	spin_unlock(&priv->lock);
>  
>  	if (use_ptemod)
>  		mmu_notifier_unregister(&priv->mn, priv->mm);
> 

Re: [Xen-devel] [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[David Vrabel]

On 08/10/15 23:14, Ben Hutchings wrote:
> On Wed, 2015-09-02 at 10:18 +0100, Ian Campbell wrote:
>> [resending to correct stable address, sorry folks]
>>
>> TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
>> ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
>> needs $something doing to it, either s/mutex/spinlock/ or (more likely)
>> backporting of 1401c00e59e too.
>>
>> Looking at LTS:
>>
>> 3.18.y:> 	>   Backported both.
>> 3.16.y:> 	>   Has backported neither
>> 3.14.y:> 	> * Only backported 30b03d05e074
>> 3.12.y:> 	>   Has backported neither
>> 3.10.y:> 	> * Only backported 30b03d05e074
>> 3.4.y:> 	>   Has backported neither
>> 3.2.y:> 	>   Has backported neither
>>
>> So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
>> backporting 1401c00e59e.
>>
>> 3.16/12/4/2 might need to be careful if they subsequently pick up 30b03d05.
> [...]
> 
> I came up with the patch below for 3.2.  Let me know if it's not
> correct.

Please just take commit 1401c00e59e instead.

David

Re: [Xen-devel] [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 1563 bytes --]

On Mon, 2015-10-12 at 11:11 +0100, David Vrabel wrote:
> On 08/10/15 23:14, Ben Hutchings wrote:
> > On Wed, 2015-09-02 at 10:18 +0100, Ian Campbell wrote:
> > > [resending to correct stable address, sorry folks]
> > > 
> > > TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
> > > ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
> > > needs $something doing to it, either s/mutex/spinlock/ or (more likely)
> > > backporting of 1401c00e59e too.
> > > 
> > > Looking at LTS:
> > > 
> > > 3.18.y:> > > > 	> > > >   Backported both.
> > > 3.16.y:> > > > 	> > > >   Has backported neither
> > > 3.14.y:> > > > 	> > > > * Only backported 30b03d05e074
> > > 3.12.y:> > > > 	> > > >   Has backported neither
> > > 3.10.y:> > > > 	> > > > * Only backported 30b03d05e074
> > > 3.4.y:> > > > 	> > > >   Has backported neither
> > > 3.2.y:> > > > 	> > > >   Has backported neither
> > > 
> > > So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
> > > backporting 1401c00e59e.
> > > 
> > > 3.16/12/4/2 might need to be careful if they subsequently pick up 30b03d05.
> > [...]
> > 
> > I came up with the patch below for 3.2.  Let me know if it's not
> > correct.
> 
> Please just take commit 1401c00e59e instead.

I couldn't 'just' take that commit; it doesn't apply cleanly.  However
I've backported it and changed 30b03d05e074 accordingly.  The two
patches are attached for your review.

Ben.

-- 
Ben Hutchings
Teamwork is essential - it allows you to blame someone else.

[-- Attachment #1.2: xen-gntdev-convert-priv-lock-to-a-mutex.patch --]
[-- Type: text/x-patch, Size: 4233 bytes --]

From: David Vrabel <david.vrabel@citrix.com>
Date: Fri, 9 Jan 2015 18:06:12 +0000
Subject: xen/gntdev: convert priv->lock to a mutex

commit 1401c00e59ea021c575f74612fe2dbba36d6a4ee upstream.

Unmapping may require sleeping and we unmap while holding priv->lock, so
convert it to a mutex.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
[bwh: Backported to 3.2:
 - Adjust context
 - Drop changes to functions we don't have]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/xen/gntdev.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -60,7 +60,7 @@ static int use_ptemod;
 struct gntdev_priv {
 	struct list_head maps;
 	/* lock protects maps from concurrent changes */
-	spinlock_t lock;
+	struct mutex lock;
 	struct mm_struct *mm;
 	struct mmu_notifier mn;
 };
@@ -395,7 +395,7 @@ static void mn_invl_range_start(struct m
 	unsigned long mstart, mend;
 	int err;
 
-	spin_lock(&priv->lock);
+	mutex_lock(&priv->lock);
 	list_for_each_entry(map, &priv->maps, next) {
 		if (!map->vma)
 			continue;
@@ -414,7 +414,7 @@ static void mn_invl_range_start(struct m
 					(mend - mstart) >> PAGE_SHIFT);
 		WARN_ON(err);
 	}
-	spin_unlock(&priv->lock);
+	mutex_unlock(&priv->lock);
 }
 
 static void mn_invl_page(struct mmu_notifier *mn,
@@ -431,7 +431,7 @@ static void mn_release(struct mmu_notifi
 	struct grant_map *map;
 	int err;
 
-	spin_lock(&priv->lock);
+	mutex_lock(&priv->lock);
 	list_for_each_entry(map, &priv->maps, next) {
 		if (!map->vma)
 			continue;
@@ -441,7 +441,7 @@ static void mn_release(struct mmu_notifi
 		err = unmap_grant_pages(map, /* offset */ 0, map->count);
 		WARN_ON(err);
 	}
-	spin_unlock(&priv->lock);
+	mutex_unlock(&priv->lock);
 }
 
 struct mmu_notifier_ops gntdev_mmu_ops = {
@@ -462,7 +462,7 @@ static int gntdev_open(struct inode *ino
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&priv->maps);
-	spin_lock_init(&priv->lock);
+	mutex_init(&priv->lock);
 
 	if (use_ptemod) {
 		priv->mm = get_task_mm(current);
@@ -535,10 +535,10 @@ static long gntdev_ioctl_map_grant_ref(s
 		return err;
 	}
 
-	spin_lock(&priv->lock);
+	mutex_lock(&priv->lock);
 	gntdev_add_map(priv, map);
 	op.index = map->index << PAGE_SHIFT;
-	spin_unlock(&priv->lock);
+	mutex_unlock(&priv->lock);
 
 	if (copy_to_user(u, &op, sizeof(op)) != 0)
 		return -EFAULT;
@@ -557,13 +557,13 @@ static long gntdev_ioctl_unmap_grant_ref
 		return -EFAULT;
 	pr_debug("priv %p, del %d+%d\n", priv, (int)op.index, (int)op.count);
 
-	spin_lock(&priv->lock);
+	mutex_lock(&priv->lock);
 	map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count);
 	if (map) {
 		list_del(&map->next);
 		err = 0;
 	}
-	spin_unlock(&priv->lock);
+	mutex_unlock(&priv->lock);
 	if (map)
 		gntdev_put_map(map);
 	return err;
@@ -608,7 +608,7 @@ static long gntdev_ioctl_notify(struct g
 	if (op.action & ~(UNMAP_NOTIFY_CLEAR_BYTE|UNMAP_NOTIFY_SEND_EVENT))
 		return -EINVAL;
 
-	spin_lock(&priv->lock);
+	mutex_lock(&priv->lock);
 
 	list_for_each_entry(map, &priv->maps, next) {
 		uint64_t begin = map->index << PAGE_SHIFT;
@@ -631,7 +631,7 @@ static long gntdev_ioctl_notify(struct g
 	map->notify.event = op.event_channel_port;
 	rc = 0;
  unlock_out:
-	spin_unlock(&priv->lock);
+	mutex_unlock(&priv->lock);
 	return rc;
 }
 
@@ -676,7 +676,7 @@ static int gntdev_mmap(struct file *flip
 	pr_debug("map %d+%d at %lx (pgoff %lx)\n",
 			index, count, vma->vm_start, vma->vm_pgoff);
 
-	spin_lock(&priv->lock);
+	mutex_lock(&priv->lock);
 	map = gntdev_find_map_index(priv, index, count);
 	if (!map)
 		goto unlock_out;
@@ -711,7 +711,7 @@ static int gntdev_mmap(struct file *flip
 			map->flags |= GNTMAP_readonly;
 	}
 
-	spin_unlock(&priv->lock);
+	mutex_unlock(&priv->lock);
 
 	if (use_ptemod) {
 		err = apply_to_page_range(vma->vm_mm, vma->vm_start,
@@ -739,11 +739,11 @@ static int gntdev_mmap(struct file *flip
 	return 0;
 
 unlock_out:
-	spin_unlock(&priv->lock);
+	mutex_unlock(&priv->lock);
 	return err;
 
 out_unlock_put:
-	spin_unlock(&priv->lock);
+	mutex_unlock(&priv->lock);
 out_put_map:
 	if (use_ptemod)
 		map->vma = NULL;

[-- Attachment #1.3: xen-gntdevt-fix-race-condition-in-gntdev_release.patch --]
[-- Type: text/x-patch, Size: 1360 bytes --]

From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Fri, 26 Jun 2015 03:28:24 +0200
Subject: xen/gntdevt: Fix race condition in gntdev_release()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 <marmarek@invisiblethingslab.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
[bwh: Backported to 3.2: adjust context]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/xen/gntdev.c | 2 ++
 1 file changed, 2 insertions(+)

--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -493,11 +493,13 @@ static int gntdev_release(struct inode *
 
 	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(map);
 	}
+	mutex_unlock(&priv->lock);
 
 	if (use_ptemod)
 		mmu_notifier_unregister(&priv->mn, priv->mm);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [Xen-devel] [linux-3.14 bisection] complete test-amd64-i386-xl-qcow2

[David Vrabel]

On 12/10/15 13:56, Ben Hutchings wrote:
> On Mon, 2015-10-12 at 11:11 +0100, David Vrabel wrote:
>> On 08/10/15 23:14, Ben Hutchings wrote:
>>> On Wed, 2015-09-02 at 10:18 +0100, Ian Campbell wrote:
>>>> [resending to correct stable address, sorry folks]
>>>>
>>>> TL;DR: Any backport of 30b03d05e074 to earlier than commit 1401c00e59e
>>>> ("xen/gntdev: convert priv->lock to a mutex", which was added in v4.0)
>>>> needs $something doing to it, either s/mutex/spinlock/ or (more likely)
>>>> backporting of 1401c00e59e too.
>>>>
>>>> Looking at LTS:
>>>>
>>>> 3.18.y:> > > > 	> > > >   Backported both.
>>>> 3.16.y:> > > > 	> > > >   Has backported neither
>>>> 3.14.y:> > > > 	> > > > * Only backported 30b03d05e074
>>>> 3.12.y:> > > > 	> > > >   Has backported neither
>>>> 3.10.y:> > > > 	> > > > * Only backported 30b03d05e074
>>>> 3.4.y:> > > > 	> > > >   Has backported neither
>>>> 3.2.y:> > > > 	> > > >   Has backported neither
>>>>
>>>> So AFAICT 3.14.y and 3.10.y need fixes, probably following 3.18 and
>>>> backporting 1401c00e59e.
>>>>
>>>> 3.16/12/4/2 might need to be careful if they subsequently pick up 30b03d05.
>>> [...]
>>>
>>> I came up with the patch below for 3.2.  Let me know if it's not
>>> correct.
>>
>> Please just take commit 1401c00e59e instead.
> 
> I couldn't 'just' take that commit; it doesn't apply cleanly.  However
> I've backported it and changed 30b03d05e074 accordingly.  The two
> patches are attached for your review.

They look fine, thanks.

David