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

[Ian Campbell <ian.campbell@citrix.com>]

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);
>