[PATCH] xen/gntdev: Fix sleep-inside-spinlock

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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