Re: [Xen-devel] race condition in xen-gntdev

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Jul 22, 2015 at 05:21:55AM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Jun 29, 2015 at 04:50:10PM +0200, Marek Marczykowski-Górecki wrote:
> > On Mon, Jun 29, 2015 at 10:39:26AM -0400, Konrad Rzeszutek Wilk wrote:
> > > 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?
> > 
> > Sure. I've already installed patched kernel, will report back results
> > later.
> 
> Ok, after few weeks I can surely confirm - this fixes the issue.

Fantastic! I think David has already committed it in.


Thanks again for doing such a detailed analysis of the issue.
> 
> > > > -----------8<------------
> > > > 
> > > > From b876e14888bdafa112c3265e6420543fa74aa709 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
> > > >  <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > > 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@xxxxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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?
> > > 
> > > 
> > 
> 
> 
> 
> -- 
> 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?



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel