Xen devel - Oct 2011 - Re: BUG: sleeping function called from invalid context

On Tue, Oct 11, 2011 at 07:13:38PM +0200, Dario Faggioli
wrote:
> Hello everyone,
> 
> Since I really plan to spend some time here, let me introduce myself
> first. My name is Dario Faggioli and I just joined the Citrix Platform
> Team in Cambridge (although I''ll be working from Italy).
I''ve some
> experience in Linux kernel (mainly scheduling) and not that much
> experience in Xen or virtualization in general, but I really want to
> learn and be able to contribute ASAP!
> 
> In fact, while "doing my homework", I stumbled against the
following
> BUG(). I''m able to reproduce it with xen-unstable and by just by
> enabling spinlock and mutex debug checks in linus'' Linux
> (65112dccf8a113737684366349d7f9ec373ddc47) _iff_ using
''tap:qcow2'' disk
> image, while no such thing happens with ''phy:''.
Aha.. I saw that at some point but never narrowed it down to the
right combination. Thank you for tracking this down.

I am CC Daniel who might have some patches for this or
ideas.
> 
> [  996.282544] BUG: sleeping function called from invalid context at
/local/scratch/dariof/linux/kernel/mutex.c:271
> [  996.282570] in_atomic(): 1, irqs_disabled(): 0, pid: 3256, name: qemu-dm
> [  996.282581] 1 lock held by qemu-dm/3256:
> [  996.282589]  #0:  (&(&priv->lock)->rlock){......}, at:
[<ffffffff813223da>] gntdev_ioctl+0x2bd/0x4d5
> [  996.282628] Pid: 3256, comm: qemu-dm Tainted: G        W   3.1.0-rc8+ #5
> [  996.282638] Call Trace:
> [  996.282687]  [<ffffffff81054594>] __might_sleep+0x131/0x135
> [  996.282704]  [<ffffffff816bd64f>] mutex_lock_nested+0x25/0x45
> [  996.282721]  [<ffffffff8131c7c8>]
free_xenballooned_pages+0x20/0xb1
> [  996.282735]  [<ffffffff8132194d>] gntdev_put_map+0xa8/0xdb
> [  996.282749]  [<ffffffff816be546>] ? _raw_spin_lock+0x71/0x7a
> [  996.282763]  [<ffffffff813223da>] ? gntdev_ioctl+0x2bd/0x4d5
> [  996.282776]  [<ffffffff8132243c>] gntdev_ioctl+0x31f/0x4d5
> [  996.282790]  [<ffffffff81007d62>] ? check_events+0x12/0x20
> [  996.282804]  [<ffffffff811433bc>] do_vfs_ioctl+0x488/0x4d7
> [  996.282818]  [<ffffffff81007d4f>] ?
xen_restore_fl_direct_reloc+0x4/0x4
> [  996.282832]  [<ffffffff8109168b>] ? lock_release+0x21c/0x229
> [  996.282847]  [<ffffffff81135cdd>] ? rcu_read_unlock+0x21/0x32
> [  996.282860]  [<ffffffff81143452>] sys_ioctl+0x47/0x6a
> [  996.282873]  [<ffffffff816bfd82>] system_call_fastpath+0x16/0x1b
> 
> This seems to be due to free_xenballooned_pages(), called by
> gntdev_put_map(), taking balloon_mutex, with the latter that can be
> called within a spin_lock() (e.g., in gntdev_release()).
> 
> I''m not enough confident with the code do attempt fixing it, but I
> thought it was worth to at least point it out!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/11/2011 01:32 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 11, 2011 at 07:13:38PM +0200, Dario Faggioli wrote:
>> Hello everyone,
>>
>> Since I really plan to spend some time here, let me introduce myself
>> first. My name is Dario Faggioli and I just joined the Citrix Platform
>> Team in Cambridge (although I''ll be working from Italy).
I''ve some
>> experience in Linux kernel (mainly scheduling) and not that much
>> experience in Xen or virtualization in general, but I really want to
>> learn and be able to contribute ASAP!
>>
>> In fact, while "doing my homework", I stumbled against the
following
>> BUG(). I''m able to reproduce it with xen-unstable and by just
by
>> enabling spinlock and mutex debug checks in linus'' Linux
>> (65112dccf8a113737684366349d7f9ec373ddc47) _iff_ using
''tap:qcow2'' disk
>> image, while no such thing happens with ''phy:''.
> 
> Aha.. I saw that at some point but never narrowed it down to the
> right combination. Thank you for tracking this down.
> 
> I am CC Daniel who might have some patches for this or ideas.
>>
>> [  996.282544] BUG: sleeping function called from invalid context at
/local/scratch/dariof/linux/kernel/mutex.c:271
>> [  996.282570] in_atomic(): 1, irqs_disabled(): 0, pid: 3256, name:
qemu-dm
>> [  996.282581] 1 lock held by qemu-dm/3256:
>> [  996.282589]  #0:  (&(&priv->lock)->rlock){......}, at:
[<ffffffff813223da>] gntdev_ioctl+0x2bd/0x4d5
>> [  996.282628] Pid: 3256, comm: qemu-dm Tainted: G        W  
3.1.0-rc8+ #5
>> [  996.282638] Call Trace:
>> [  996.282687]  [<ffffffff81054594>] __might_sleep+0x131/0x135
>> [  996.282704]  [<ffffffff816bd64f>] mutex_lock_nested+0x25/0x45
>> [  996.282721]  [<ffffffff8131c7c8>]
free_xenballooned_pages+0x20/0xb1
>> [  996.282735]  [<ffffffff8132194d>] gntdev_put_map+0xa8/0xdb
>> [  996.282749]  [<ffffffff816be546>] ? _raw_spin_lock+0x71/0x7a
>> [  996.282763]  [<ffffffff813223da>] ? gntdev_ioctl+0x2bd/0x4d5
>> [  996.282776]  [<ffffffff8132243c>] gntdev_ioctl+0x31f/0x4d5
>> [  996.282790]  [<ffffffff81007d62>] ? check_events+0x12/0x20
>> [  996.282804]  [<ffffffff811433bc>] do_vfs_ioctl+0x488/0x4d7
>> [  996.282818]  [<ffffffff81007d4f>] ?
xen_restore_fl_direct_reloc+0x4/0x4
>> [  996.282832]  [<ffffffff8109168b>] ? lock_release+0x21c/0x229
>> [  996.282847]  [<ffffffff81135cdd>] ? rcu_read_unlock+0x21/0x32
>> [  996.282860]  [<ffffffff81143452>] sys_ioctl+0x47/0x6a
>> [  996.282873]  [<ffffffff816bfd82>]
system_call_fastpath+0x16/0x1b
>>
>> This seems to be due to free_xenballooned_pages(), called by
>> gntdev_put_map(), taking balloon_mutex, with the latter that can be
>> called within a spin_lock() (e.g., in gntdev_release()).
>>
>> I''m not enough confident with the code do attempt fixing it,
but I
>> thought it was worth to at least point it out!
> 
It looks like gntdev_release doesn''t need to take the spinlock at all,
since
it''s a per-file lock on the file''s release path. The unmap
ioctl looks like
it''ll also trigger this message; that''s also fixed here.
Compile tested only.

-------------------------------------------------------->8

gntdev_put_map tries to acquire a mutex when freeing pages back to the
xenballoon pool, so it cannot be called with a spinlock held. In
gntdev_release, the spinlock is not needed as we are freeing the
structure later; in the ioctl, only the list manipulation needs to be
under the lock.

Reported-By: Dario Faggioli <raistlin@linux.it>
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntdev.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index f914b26..23b1c83 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -462,13 +462,11 @@ 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(map);
 	}
-	spin_unlock(&priv->lock);
 
 	if (use_ptemod)
 		mmu_notifier_unregister(&priv->mn, priv->mm);
@@ -532,10 +530,11 @@ static long gntdev_ioctl_unmap_grant_ref(struct
gntdev_priv *priv,
 	map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count);
 	if (map) {
 		list_del(&map->next);
-		gntdev_put_map(map);
 		err = 0;
 	}
 	spin_unlock(&priv->lock);
+	if (map)
+		gntdev_put_map(map);
 	return err;
 }
 
-- 
1.7.6.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Oct 11, 2011 at 03:16:06PM -0400, Daniel De Graaf
wrote:
> On 10/11/2011 01:32 PM, Konrad Rzeszutek Wilk wrote:
> > On Tue, Oct 11, 2011 at 07:13:38PM +0200, Dario Faggioli wrote:
> >> Hello everyone,
Dario, does the patch fix the problem for you?

.. snip..
> it''ll also trigger this message; that''s also fixed here.
Compile tested only.
> 
> -------------------------------------------------------->8
> 
> gntdev_put_map tries to acquire a mutex when freeing pages back to the
> xenballoon pool, so it cannot be called with a spinlock held. In
> gntdev_release, the spinlock is not needed as we are freeing the
> structure later; in the ioctl, only the list manipulation needs to be
> under the lock.
> 
> Reported-By: Dario Faggioli <raistlin@linux.it>
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/gntdev.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index f914b26..23b1c83 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -462,13 +462,11 @@ 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(map);
>  	}
> -	spin_unlock(&priv->lock);
>  
>  	if (use_ptemod)
>  		mmu_notifier_unregister(&priv->mn, priv->mm);
> @@ -532,10 +530,11 @@ static long gntdev_ioctl_unmap_grant_ref(struct
gntdev_priv *priv,
>  	map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT,
op.count);
>  	if (map) {
>  		list_del(&map->next);
> -		gntdev_put_map(map);
>  		err = 0;
>  	}
>  	spin_unlock(&priv->lock);
> +	if (map)
> +		gntdev_put_map(map);
>  	return err;
>  }
>  
> -- 
> 1.7.6.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, Oct 14, 2011 at 11:57:05AM +0200, Dario Faggioli
wrote:
> On Tue, 2011-10-11 at 15:16 -0400, Daniel De Graaf wrote:
> > gntdev_put_map tries to acquire a mutex when freeing pages back to the
> > xenballoon pool, so it cannot be called with a spinlock held. In
> > gntdev_release, the spinlock is not needed as we are freeing the
> > structure later; in the ioctl, only the list manipulation needs to be
> > under the lock.
> > 
> > Reported-By: Dario Faggioli <raistlin@linux.it>
> > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> >
> Ok, I finally manage in testing this, and it actually cures the issue
> here!
> 
> You can add a tested-by tag if you like. Just, if you don''t mind,
use my
> corp. e-mail for (both?) the tag(s): <dario.faggioli@citrix.com>
Sure. Thx for checking it.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel