cgroup: protect modifications to cgroup->idr with cgroup_mutex

Setup cgroupfs like this:

  # mount -t cgroup -o cpuacct xxx /cgroup
  # mkdir /cgroup/sub1
  # mkdir /cgroup/sub2

Then run these two commands:

  # for ((; ;)) { mkdir /cgroup/sub1/tmp && rmdir /cgroup/sub1/tmp; } &
  # for ((; ;)) { mkdir /cgroup/sub2/tmp && rmdir /cgroup/sub2/tmp; } &

After seconds you may see this warning:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 25243 at lib/idr.c:527 sub_remove+0x87/0x1b0()
idr_remove called for id=6 which is not allocated.
...
Call Trace:
 [<ffffffff8156063c>] dump_stack+0x7a/0x96
 [<ffffffff810591ac>] warn_slowpath_common+0x8c/0xc0
 [<ffffffff81059296>] warn_slowpath_fmt+0x46/0x50
 [<ffffffff81300aa7>] sub_remove+0x87/0x1b0
 [<ffffffff810f3f02>] ? css_killed_work_fn+0x32/0x1b0
 [<ffffffff81300bf5>] idr_remove+0x25/0xd0
 [<ffffffff810f2bab>] cgroup_destroy_css_killed+0x5b/0xc0
 [<ffffffff810f4000>] css_killed_work_fn+0x130/0x1b0
 [<ffffffff8107cdbc>] process_one_work+0x26c/0x550
 [<ffffffff8107eefe>] worker_thread+0x12e/0x3b0
 [<ffffffff81085f96>] kthread+0xe6/0xf0
 [<ffffffff81570bac>] ret_from_fork+0x7c/0xb0
---[ end trace 2d1577ec10cf80d0 ]---

It's because allocating/removing cgroup ID is not properly synchronized.

The bug was introduced when we converted cgroup_ida to cgroup_idr. While synchronization is already done inside ida_simple_{get,remove}(), users are responsible for concurrent calls to idr_{alloc,remove}().

v2:
- Don't call deactivate_super() inside cgroup_mutex, as cgroup_kill_sb() will be called if sb refcnt reaches 0. I don't think this can happen, as cgroup_create() is called through vfs, so vfs should guarantee the superblock won't disappear. Still better not depend on it even my guess is probably correct.

---

Also rebased on "cgroup: fix error return from cgroup_create()".

---
 include/linux/cgroup.h |  2 ++
 kernel/cgroup.c        | 47 ++++++++++++++++++++++++++---------------------
 2 files changed, 28 insertions(+), 21 deletions(-)

index 5c09759..9450f02 100644
@@ -166,6 +166,8 @@ struct cgroup {
 	 *
 	 * The ID of the root cgroup is always 0, and a new cgroup
 	 * will be assigned with a smallest available ID.
+	 *
+	 * Allocating/Removing ID must be protected by cgroup_mutex.
 	 */
 	int id;
 

1.8.0.2

Hello, Li.

If the deadlock can't actually happen, I don't really care either way as the code goes away after kernfs conversion anyway. I've already applied v1, so if you think this change is important, can you send an incremental patch?

Thanks!

tejun

I'm fine to stick with V1.

I'm pretty confident it's safe, as we can increment sb refcnt without any checking or locking (even cgroup_mutex as the comment says) in cgroup_create().