Re: race condition in xen-gntdev

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Jun 26, 2015 at 03:28:24AM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Jun 22, 2015 at 03:14:16PM -0400, Daniel De Graaf wrote:
> > The reason that gntdev_release didn't have a lock is because there are not
> > supposed to be any references to the areas pointed to by priv->maps when it
> > is called.  However, since the MMU notifier has not yet been unregistered,
> > it is apparently possible to race here; the comment on mmu_notifier_unregister
> > seems to confirm this as a possibility (as do the backtraces).
> > 
> > I think adding the lock will be sufficient.
> 
> Ok, so here is the patch:

Awesome!

Since you are the one who has been seeing this particular fault - any chance
you could give it some soak time? If I recall your emails correctly it takes
about a week or so before you saw the crash?

> 
> -----------8<------------
> 
> From b876e14888bdafa112c3265e6420543fa74aa709 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
>  <marmarek@invisiblethingslab.com>
> Date: Fri, 26 Jun 2015 02:16:49 +0200
> Subject: [PATCH] xen/grant: fix race condition in gntdev_release
> 
> While gntdev_release is called, 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>
> ---
>  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);
>  
> +	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);
> -- 
> 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?