Bug 12673 - possible recursive locking detected in sget()
Summary: possible recursive locking detected in sget()
Status: CLOSED PATCH_ALREADY_AVAILABLE
Alias: None
Product: File System
Classification: Unclassified
Component: VFS (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: fs_vfs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-09 00:10 UTC by Li Zefan
Modified: 2009-02-18 19:03 UTC (History)
2 users (show)

See Also:
Kernel Version: 2.6.29-rc4
Subsystem:
Regression: ---
Bisected commit-id:


Attachments

Description Li Zefan 2009-02-09 00:10:51 UTC
kernel version: 2.6.29-rc4

Problem Description and steps to reproduce:

Thread 1:
  for ((; ;))
  {
      mount -t cpuset xxx /mnt > /dev/null 2>&1
      cat /mnt/cpus > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1
  }

Thread 2:
  for ((; ;))
  {
      mount -t cpuset xxx /mnt > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1
  }

(Note: It is irrelevant which cgroup subsys is used.)

After a while a lockdep warning showed up:

=============================================
[ INFO: possible recursive locking detected ]
2.6.29-rc4 #544
---------------------------------------------
mount/5396 is trying to acquire lock:
 (&type->s_umount_key#19){--..}, at: [<c04a3f72>] sget+0x58/0x31f

but task is already holding lock:
 (&type->s_umount_key#19){--..}, at: [<c04a40ff>] sget+0x1e5/0x31f

other info that might help us debug this:
1 lock held by mount/5396:
 #0:  (&type->s_umount_key#19){--..}, at: [<c04a40ff>] sget+0x1e5/0x31f

stack backtrace:
Pid: 5396, comm: mount Not tainted 2.6.29-rc4 #544
Call Trace:
 [<c044e208>] validate_chain+0x4c6/0xbbd
 [<c0427987>] ? finish_task_switch+0x4e/0xa2
 [<c044ef75>] __lock_acquire+0x676/0x700
 [<c044f05c>] lock_acquire+0x5d/0x7a
 [<c04a3f72>] ? sget+0x58/0x31f
 [<c0626269>] down_write+0x34/0x50
 [<c04a3f72>] ? sget+0x58/0x31f
 [<c04a3f72>] sget+0x58/0x31f
 [<c045dda9>] ? cgroup_set_super+0x0/0x3e
 [<c045cf6f>] ? cgroup_test_super+0x0/0x2f
 [<c045f840>] cgroup_get_sb+0x8d/0x284
 [<c0489216>] ? kstrdup+0x31/0x53
 [<c04a46a5>] vfs_kern_mount+0x40/0x7b
 [<c04a472e>] do_kern_mount+0x37/0xbf
 [<c04b5dc2>] do_mount+0x5c4/0x61b
 [<c04b4776>] ? copy_mount_options+0x2c/0x111
 [<c04b5e82>] sys_mount+0x69/0xa0
 [<c0403351>] sysenter_do_call+0x12/0x31

The cause is after alloc_super() and then retry, an old entry in list
fs_supers is found, so grab_super(old) is called, but both functions
hold s_umount lock:

struct super_block *sget(...)
{
	...
retry:
	spin_lock(&sb_lock);
	if (test) {
		list_for_each_entry(old, &type->fs_supers, s_instances) {
			if (!test(old, data))
				continue;
			if (!grab_super(old))  <--- 2nd: down_write(&old->s_umount);
				goto retry;
			if (s)
				destroy_super(s);
			return old;
		}
	}
	if (!s) {
		spin_unlock(&sb_lock);
		s = alloc_super(type);   <--- 1th: down_write(&s->s_umount)
		if (!s)
			return ERR_PTR(-ENOMEM);
		goto retry;
	}
	...
}

It seems like a false positive, and seems like VFS but not cgroup needs
to be fixed ?

And I noticed this commit:

commit 897c6ff9568bcb102ffc6b465ebe1def0cba829d
Author: Arjan van de Ven <arjan@infradead.org>
Date:   Mon Jul 3 00:25:28 2006 -0700

    [PATCH] lockdep: annotate sb ->s_umount

    The s_umount rwsem needs to be classified as per-superblock since it's
    perfectly legit to keep multiple of those recursively in the VFS locking
    rules.

    Has no effect on non-lockdep kernels.

The changelog said s_umount needs to be classified as per-sb, but actually
it made it as per-filesystem. And there is no way to mark all instances
of a given lock as distinct.
Comment 1 Andrew Morton 2009-02-09 00:16:49 UTC
It'd be better to handle this bug by the usual email means, IMO.
Please cc Arjan, linux-kernel and the containers list?
Comment 2 Li Zefan 2009-02-09 00:24:38 UTC
That's what I did, but no one responded to my reports. :(

http://lkml.org/lkml/2009/1/4/354
http://lkml.org/lkml/2009/1/4/352

so I turned to the bugzilla to track these 2 issues.
Comment 3 Li Zefan 2009-02-09 00:29:29 UTC
I think I can silence this lockdep warning with the following patch, but I don't think
it's a real fix.

(patch not tested)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 5a54ff4..52cf329 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -50,6 +50,7 @@
 #include <asm/atomic.h>
 
 static DEFINE_MUTEX(cgroup_mutex);
+static DEFINE_MUTEX(cgroup_mount_mutex);
 
 /* Generate an array of cgroup subsystem pointers */
 #define SUBSYS(_x) &_x ## _subsys,
@@ -989,7 +990,9 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		kfree(opts.release_agent);
 	}
 
+	mutex_lock(&cgroup_mount_mutex);
 	sb = sget(fs_type, cgroup_test_super, cgroup_set_super, root);
+	mutex_unlock(&cgroup_mount_mutex);
 
 	if (IS_ERR(sb)) {
 		kfree(root);
Comment 4 Li Zefan 2009-02-11 17:14:24 UTC
patch available:

http://lkml.org/lkml/2009/2/9/135

Andrew, could you pick it up?
Comment 5 Li Zefan 2009-02-18 19:03:04 UTC
Fixed by ada723dcd681e2dffd7d73345cc8fda0eb0df9bd. Close it.

Note You need to log in before you can comment on or make changes to this bug.