Bug in cgroup_do_mount()?

Bug in cgroup_do_mount()?

[David Howells]

I'm having a go at converting cgroups to use my fs_context stuff, and I've
spotted a potential bug in the existing code.  In cgroup_do_mount(), there is
the following snippet:

		mutex_lock(&cgroup_mutex);
		spin_lock_irq(&css_set_lock);

		cgrp = cset_cgroup_from_root(ns->root_cset, root);

		spin_unlock_irq(&css_set_lock);
		mutex_unlock(&cgroup_mutex);

		nsdentry = kernfs_node_dentry(cgrp->kn, dentry->d_sb);

Given that locks must be taken to call cset_cgroup_from_root(), is the value
of cgrp trustworthy once the locks are dropped?

David

Re: Bug in cgroup_do_mount()?

[Tejun Heo]

Hello, David.

On Tue, Jun 13, 2017 at 03:27:27PM +0100, David Howells wrote:
> I'm having a go at converting cgroups to use my fs_context stuff, and I've
> spotted a potential bug in the existing code.  In cgroup_do_mount(), there is
> the following snippet:
> 
> 		mutex_lock(&cgroup_mutex);
> 		spin_lock_irq(&css_set_lock);
> 
> 		cgrp = cset_cgroup_from_root(ns->root_cset, root);
> 
> 		spin_unlock_irq(&css_set_lock);
> 		mutex_unlock(&cgroup_mutex);
> 
> 		nsdentry = kernfs_node_dentry(cgrp->kn, dentry->d_sb);
> 
> Given that locks must be taken to call cset_cgroup_from_root(), is the value
> of cgrp trustworthy once the locks are dropped?

Heh, yeah, that does look suspicious.  The css_set_lock is protecting
the list structure itself and cgroup_mutex is required to exclude
cgroup destruction and migration; however, the ns is pinning the
root_cset which is immutable and in turn pins the cgroups which are
associated with it, so the cgroup will stay pinned even after the
mutex is dropped.

The mutex lock there is to satisfy the lockdep assertion inside
cset_cgroup_from_root().  We definitely should add a comment there
explaining what's going on.

Thanks.

-- 
tejun