cgroup: fix broken css_has_online_children()

Quoted 4 lines · Li Zefan

kernel/cgroup.c +0 -0

@@ -1677,6 +1679,22 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 		goto out_unlock;
 	}
 

kernel/cgroup.c static struct dentry *cgroup_mount(struct file_system_type *fs_type, · line 1690

Quoted 9 lines

kernel/cgroup.c +9 -0

+	/*
+	 * If some subsystems have been bound to existing cgroup hierarchies,
+	 * but those hierachies are being destroyed, let's wait a little bit
+	 * and retry.
+	 */
+	for_each_subsys(ss, i) {
+		if (!(opts.subsys_mask & (1 << i)))
+			continue;
+		if (!percpu_ref_alive(&ss->root->cgrp.self.refcnt)) {

Can't we just do tryget_live() instead and then put before retrying? It's not exactly a hot path and the operations are dirt cheap anyway.

We've converted cgroup to kernfs so cgroup won't be intertwined with vfs objects and locking, but there are dark areas.

Run two instances of this script concurrently:

    for ((; ;))
    {
    	mount -t cgroup -o cpuacct xxx /cgroup
    	umount /cgroup
    }

After a while, I saw two mount processes were stuck at retrying, because they were waiting for a subsystem to become free, but the root associated with this subsystem never got freed.

This can happen, if thread A is in the process of killing superblock but hasn't called percpu_ref_kill(), and at this time thread B is mounting the same cgroup root and finds the root in the root list and performs percpu_ref_try_get().

To fix this, we increase the refcnt of the superblock instead of increasing the percpu refcnt of cgroup root.

Hello, Li.

Sorry about the long delay.

Quoted 15 lines · Li Zefan

We've converted cgroup to kernfs so cgroup won't be intertwined with vfs objects and locking, but there are dark areas.

Run two instances of this script concurrently:

    for ((; ;))
    {
    	mount -t cgroup -o cpuacct xxx /cgroup
    	umount /cgroup
    }

After a while, I saw two mount processes were stuck at retrying, because they were waiting for a subsystem to become free, but the root associated with this subsystem never got freed.

Quoted 7 lines

This can happen, if thread A is in the process of killing superblock but hasn't called percpu_ref_kill(), and at this time thread B is mounting the same cgroup root and finds the root in the root list and performs percpu_ref_try_get().

To fix this, we increase the refcnt of the superblock instead of increasing the percpu refcnt of cgroup root.

Ah, right. Gees, I'm really hating the fact that we have ->mount but not ->umount. However, can't we make it a bit simpler by just introducing a mutex protecting looking up and refing up an existing root and a sb going away? The only problem is that the refcnt being killed isn't atomic w.r.t. new live ref coming up, right? Why not just add a mutex around them so that they can't race?

Quoted 6 lines

Ah, right. Gees, I'm really hating the fact that we have ->mount but not ->umount. However, can't we make it a bit simpler by just introducing a mutex protecting looking up and refing up an existing root and a sb going away? The only problem is that the refcnt being killed isn't atomic w.r.t. new live ref coming up, right? Why not just add a mutex around them so that they can't race?

Well, kill_sb() is called with sb->s_umount held, while kernfs_mount() returned with sb->s_umount held, so adding a mutex will lead to ABBA deadlock.

Hello, Li.

Quoted 7 lines · Li Zefan

Quoted 6 lines

Ah, right. Gees, I'm really hating the fact that we have ->mount but not ->umount. However, can't we make it a bit simpler by just introducing a mutex protecting looking up and refing up an existing root and a sb going away? The only problem is that the refcnt being killed isn't atomic w.r.t. new live ref coming up, right? Why not just add a mutex around them so that they can't race?

Quoted 3 lines

Well, kill_sb() is called with sb->s_umount held, while kernfs_mount() returned with sb->s_umount held, so adding a mutex will lead to ABBA deadlock.

Hmmm? Why does that matter? The only region in cgroup_mount() which needs to be put inside such mutex would be root lookup, no?

Quoted 2 lines

Hmmm? Why does that matter? The only region in cgroup_mount() which needs to be put inside such mutex would be root lookup, no?

unfortunately that won't help. I think what you suggest is:

cgroup_mount()

{
	mutex_lock();
	lookup_cgroup_root();
	mutex_unlock();
	kernfs_mount();
}

cgroup_kill_sb()

{
	mutex_lock();
	percpu_ref_kill();
	mutex_Unlock();
	kernfs_kill_sb();
}

See, we may still be destroying the superblock after we've succeeded in getting the refcnt of cgroup root.

Hey,

Quoted 21 lines · Li Zefan

Quoted 2 lines

Hmmm? Why does that matter? The only region in cgroup_mount() which needs to be put inside such mutex would be root lookup, no?

unfortunately that won't help. I think what you suggest is:

cgroup_mount()

{
	mutex_lock();
	lookup_cgroup_root();
	mutex_unlock();
	kernfs_mount();
}

cgroup_kill_sb()

{
	mutex_lock();
	percpu_ref_kill();
	mutex_Unlock();
	kernfs_kill_sb();
}

Quoted 2 lines

See, we may still be destroying the superblock after we've succeeded in getting the refcnt of cgroup root.

Sure, but now the decision to kill is synchronized so the other side can interlock with it. e.g.

cgroup_mount()

{
	mutex_lock();
	lookup_cgroup_root();
	if (root isn't killed yet)
		root->this_better_stay_alive++;
	mutex_unlock();
	kernfs_mount();
}

cgroup_kill_sb()

{
	mutex_lock();
	if (check whether root can be killed)
		percpu_ref_kill();
	mutex_unlock();
	if (the above condition was true)
		kernfs_kill_sb();
}

Quoted 11 lines

cgroup_mount()

{
	mutex_lock();
	lookup_cgroup_root();
	if (root isn't killed yet)
		root->this_better_stay_alive++;
	mutex_unlock();
	kernfs_mount();
}

cgroup_kill_sb()

Quoted 9 lines

{
	mutex_lock();
	if (check whether root can be killed)
		percpu_ref_kill();
	mutex_unlock();
	if (the above condition was true)
		kernfs_kill_sb();
}

This looks nasty, and I don't think it's correct. If we skip the call to kernfs_kill_sb(), kernfs_super_info won't be freed but super_block will, so we will end up either leaking memory or accessing invalid memory. There are other problems like returning with sb->s_umount still held.

After running:

  # mount -t cgroup cpu xxx /cgroup && mkdir /cgroup/sub && \
    rmdir /cgroup/sub && umount /cgroup

I found the cgroup root still existed:

  # cat /proc/cgroups
  #subsys_name    hierarchy       num_cgroups     enabled
  cpuset  0       1       1
  cpu     1       1       1
  ...

It turned out css_has_online_children() is broken.

Signed-off-by Li Zefan <[email protected]>