[PATCH] cgroup: Show control files in cgroup2 root after mount

[PATCH] cgroup: Show control files in cgroup2 root after mount

[Waiman Long]

It was found that when a cgroup2 filesystem was mounted, control
files other than the base cgroup.* ones were not shown in the root
directory.  They were shown only after some controllers were activated
in the root's cgroup.subtree_control file.

This was caused by a lack of the kernfs_activate() call which was fixed
by this patch.

Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup/cgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 89a23c6..fb1893b 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2024,8 +2024,10 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 
 		dentry = cgroup_do_mount(&cgroup2_fs_type, flags, &cgrp_dfl_root,
 					 CGROUP2_SUPER_MAGIC, ns);
-		if (!IS_ERR(dentry))
+		if (!IS_ERR(dentry)) {
 			apply_cgroup_root_flags(root_flags);
+			kernfs_activate(cgrp_dfl_root.cgrp.kn);
+		}
 	} else {
 		dentry = cgroup1_mount(&cgroup_fs_type, flags, data,
 				       CGROUP_SUPER_MAGIC, ns);
-- 
1.8.3.1

Re: [PATCH] cgroup: Show control files in cgroup2 root after mount

[Tejun Heo]

Hello, Waiman.

On Tue, Jul 18, 2017 at 03:32:16PM -0400, Waiman Long wrote:
> It was found that when a cgroup2 filesystem was mounted, control
> files other than the base cgroup.* ones were not shown in the root
> directory.  They were shown only after some controllers were activated
> in the root's cgroup.subtree_control file.
> 
> This was caused by a lack of the kernfs_activate() call which was fixed
> by this patch.
> 
> Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  kernel/cgroup/cgroup.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 89a23c6..fb1893b 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2024,8 +2024,10 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
>  
>  		dentry = cgroup_do_mount(&cgroup2_fs_type, flags, &cgrp_dfl_root,
>  					 CGROUP2_SUPER_MAGIC, ns);
> -		if (!IS_ERR(dentry))
> +		if (!IS_ERR(dentry)) {
>  			apply_cgroup_root_flags(root_flags);
> +			kernfs_activate(cgrp_dfl_root.cgrp.kn);
> +		}

Heh, that's tricky.  I'm not quite sure where the unactivated files
are being added tho because that'd be where we should be activating.
I *think* that they are already activated as part of
cgroup_add_cftypes() but I am obviously qmissing something.  I'll try
to repro the issue and find where we're skipping the activation call.

Thanks!

-- 
tejun

Re: [PATCH] cgroup: Show control files in cgroup2 root after mount

[Waiman Long]

On 07/18/2017 03:51 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Tue, Jul 18, 2017 at 03:32:16PM -0400, Waiman Long wrote:
>> It was found that when a cgroup2 filesystem was mounted, control
>> files other than the base cgroup.* ones were not shown in the root
>> directory.  They were shown only after some controllers were activated
>> in the root's cgroup.subtree_control file.
>>
>> This was caused by a lack of the kernfs_activate() call which was fixed
>> by this patch.
>>
>> Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  kernel/cgroup/cgroup.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>> index 89a23c6..fb1893b 100644
>> --- a/kernel/cgroup/cgroup.c
>> +++ b/kernel/cgroup/cgroup.c
>> @@ -2024,8 +2024,10 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
>>  
>>  		dentry = cgroup_do_mount(&cgroup2_fs_type, flags, &cgrp_dfl_root,
>>  					 CGROUP2_SUPER_MAGIC, ns);
>> -		if (!IS_ERR(dentry))
>> +		if (!IS_ERR(dentry)) {
>>  			apply_cgroup_root_flags(root_flags);
>> +			kernfs_activate(cgrp_dfl_root.cgrp.kn);
>> +		}
> Heh, that's tricky.  I'm not quite sure where the unactivated files
> are being added tho because that'd be where we should be activating.
> I *think* that they are already activated as part of
> cgroup_add_cftypes() but I am obviously qmissing something.  I'll try
> to repro the issue and find where we're skipping the activation call.
>
> Thanks!
>
From my own debugging, the controller files (e.g. the debug controller)
were indirectly populated by the rebind_subsystems() call.

[    1.628103] css_populate_dir: init subsystem debug
[    1.628944] ------------[ cut here ]------------
[    1.629796] WARNING: CPU: 0 PID: 1 at kernel/cgroup/cgroup.c:1584
css_populate_dir+0x141/0x180
[    1.631401] Modules linked in:
[    1.632052] CPU: 0 PID: 1 Comm: systemd Tainted: G        W      
4.13.0-rc1-cgroup2+ #13
[    1.633642] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
[    1.634627] task: ffff91d84e1f0000 task.stack: ffffb40b8189c000
[    1.635630] RIP: 0010:css_populate_dir+0x141/0x180
[    1.636548] RSP: 0018:ffffb40b8189fb88 EFLAGS: 00010246
[    1.637457] RAX: 0000000000000026 RBX: ffff91d84fcb7ec0 RCX:
ffffffffaec60ae8
[    1.638538] RDX: 0000000000000000 RSI: 0000000000000082 RDI:
0000000000000246
[    1.639689] RBP: ffffb40b8189fbb0 R08: 0000000000000000 R09:
0000000000000325
[    1.640889] R10: 00000000ffffffff R11: 0000000000000324 R12:
ffffffffaecec3f8
[    1.642071] R13: ffff91d84fcb7ec0 R14: ffffffffaf1f6770 R15:
ffffffffaeceea60
[    1.643263] FS:  00007fc8c9624940(0000) GS:ffff91dad7200000(0000)
knlGS:0000000000000000
[    1.651469] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.653184] CR2: 00007fc8c9630000 CR3: 0000000190bff000 CR4:
00000000001406f0
[    1.654401] Call Trace:
[    1.654975]  cgroup_apply_control_enable+0x103/0x340
[    1.655906]  ? css_next_descendant_pre+0x35/0x40
[    1.656834]  ? cgroup_propagate_control+0x101/0x150
[    1.657719]  cgroup_apply_control+0x1a/0x30
[    1.658521]  rebind_subsystems+0x18a/0x3b0
[    1.659272]  cgroup_setup_root+0x18f/0x380
[    1.660086]  cgroup1_mount+0x2c5/0x490
[    1.660842]  cgroup_mount+0x9d/0x390
[    1.661586]  mount_fs+0x39/0x150
[    1.662238]  vfs_kern_mount+0x67/0x130
[    1.663027]  do_mount+0x1e2/0xc90
[    1.663718]  ? kmem_cache_alloc_trace+0x14b/0x1b0
[    1.664548]  SyS_mount+0x83/0xd0
[    1.665254]  entry_SYSCALL_64_fastpath+0x1a/0xa5

For the default cgroup2 root, kernfs_activate() was only called at the
beginning in cgroup_init() with only the base cgroup files added. No
more call after that until I touched the cgroup.subtree_control file.

Cheers,
Longman

Re: [PATCH] cgroup: Show control files in cgroup2 root after mount

[Tejun Heo]

Hello,

On Tue, Jul 18, 2017 at 04:00:45PM -0400, Waiman Long wrote:
> From my own debugging, the controller files (e.g. the debug controller)
> were indirectly populated by the rebind_subsystems() call.
> 
> [    1.628103] css_populate_dir: init subsystem debug
...
> [    1.654975]  cgroup_apply_control_enable+0x103/0x340
> [    1.657719]  cgroup_apply_control+0x1a/0x30
> [    1.658521]  rebind_subsystems+0x18a/0x3b0
...

But there's kernfs_activate() call at the end of rebind_subsystems(),
so if the files were being added there, it should have been activated
there and I can confirm that the files are correctly added / removed
from the cgroup2 root directory when controllers are attached to /
detached from it.

> For the default cgroup2 root, kernfs_activate() was only called at the
> beginning in cgroup_init() with only the base cgroup files added. No
> more call after that until I touched the cgroup.subtree_control file.

Hmm... we're activating at the end of

* cgroup_setup_root()
* rebind_subsystems()
* cgroup_subtree_control_write()
* cgroup_apply_cftypes() after successful addition
* cgroup_mkdir()

I *think* this should cover everything.  Just in case, are you looking
at the mainline kernel?  Can you share how you can reproduce the
issue?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: Show control files in cgroup2 root after mount

[Waiman Long]

On 07/18/2017 04:12 PM, Tejun Heo wrote:
> Hello,
>
> On Tue, Jul 18, 2017 at 04:00:45PM -0400, Waiman Long wrote:
>> From my own debugging, the controller files (e.g. the debug controller)
>> were indirectly populated by the rebind_subsystems() call.
>>
>> [    1.628103] css_populate_dir: init subsystem debug
> ...
>> [    1.654975]  cgroup_apply_control_enable+0x103/0x340
>> [    1.657719]  cgroup_apply_control+0x1a/0x30
>> [    1.658521]  rebind_subsystems+0x18a/0x3b0
> ...
>
> But there's kernfs_activate() call at the end of rebind_subsystems(),
> so if the files were being added there, it should have been activated
> there and I can confirm that the files are correctly added / removed
> from the cgroup2 root directory when controllers are attached to /
> detached from it.

I think the kernfs_activate() call is for the cgroup1 mount of debug
cgroup which probably failed as I put debug in the cgroup_no_v1= option.
The RHEL7 system that I ran the test on tried to do v1-mount of all the
cgroup controllers available. I am also wondering how a v1-mount of
debug controller will make the controller files appear on cgroup2 root.
Maybe I miss something in the code.

>> For the default cgroup2 root, kernfs_activate() was only called at the
>> beginning in cgroup_init() with only the base cgroup files added. No
>> more call after that until I touched the cgroup.subtree_control file.
> Hmm... we're activating at the end of
>
> * cgroup_setup_root()
> * rebind_subsystems()
> * cgroup_subtree_control_write()
> * cgroup_apply_cftypes() after successful addition
> * cgroup_mkdir()
>
> I *think* this should cover everything.  Just in case, are you looking
> at the mainline kernel?  Can you share how you can reproduce the
> issue?

My test kernel was built out of your latest cgroup git tree with the
thread mode patches on (review-cgroup2-threads-v3 branch).

As I said above, I put in the kernel command line option
"cgroup_no_v1=pids,debug,memory". Then I mounted the cgroup2 filesystem
after boot.

# mount -t cgroup2 cgroup2 /cgroup2
# ls /cgroup2/
cgroup.controllers  cgroup.procs  cgroup.subtree_control  cgroup.threads
# echo +memory > /cgroup2/cgroup.subtree_control
# ls /cgroup2/
cgroup.controllers    debug.current_css_set
cgroup.procs        debug.current_css_set_cg_links
cgroup.subtree_control    debug.current_css_set_refcount
cgroup.threads        debug.masks
debug.csses        debug.taskcount
debug.css_links

Cheers,
Longman

Re: [PATCH] cgroup: Show control files in cgroup2 root after mount

[Tejun Heo]

Hello,

On Tue, Jul 18, 2017 at 04:29:40PM -0400, Waiman Long wrote:
> I think the kernfs_activate() call is for the cgroup1 mount of debug

kernfs_activate() there is for any files which are created while a
controller moves between hierarchies, so, yeah, during boot, this is
where the root files for cgroup1 hierarchies would be activated.

> cgroup which probably failed as I put debug in the cgroup_no_v1= option.

Yeah, they just get skipped over.

> The RHEL7 system that I ran the test on tried to do v1-mount of all the
> cgroup controllers available. I am also wondering how a v1-mount of
> debug controller will make the controller files appear on cgroup2 root.
> Maybe I miss something in the code.

When a controller gets mounted on v1, the controller is detached from
cgroup2 hierarchy, so the corresponding files are removed from v2 root
and then created on v1, which are activated by the kernfs_activate()
call at the end of rebind.

> My test kernel was built out of your latest cgroup git tree with the
> thread mode patches on (review-cgroup2-threads-v3 branch).

I see.  I'm testing with the same kernel.

> As I said above, I put in the kernel command line option
> "cgroup_no_v1=pids,debug,memory". Then I mounted the cgroup2 filesystem
> after boot.
> 
> # mount -t cgroup2 cgroup2 /cgroup2
> # ls /cgroup2/
> cgroup.controllers  cgroup.procs  cgroup.subtree_control  cgroup.threads
> # echo +memory > /cgroup2/cgroup.subtree_control
> # ls /cgroup2/
> cgroup.controllers    debug.current_css_set
> cgroup.procs        debug.current_css_set_cg_links
> cgroup.subtree_control    debug.current_css_set_refcount
> cgroup.threads        debug.masks
> debug.csses        debug.taskcount
> debug.css_links

Heh, I'm really confused.  If I do the same thing, I get the following
result which makes sense as the files would be activated in
cgroup_apply_cftypes() when the debug controller is registered during
boot.

 # mkdir /cgroup2
 # mount -t cgroup2 cgroup2 /cgroup2
 # ls /cgroup2/
 cgroup.controllers      debug.current_css_set
 cgroup.procs            debug.current_css_set_cg_links
 cgroup.subtree_control  debug.current_css_set_refcount
 cgroup.threads          debug.masks
 debug.csses             debug.taskcount
 debug.css_links

Thanks.

-- 
tejun

Re: [PATCH] cgroup: Show control files in cgroup2 root after mount

[Tejun Heo]

On Tue, Jul 18, 2017 at 04:50:10PM -0400, Tejun Heo wrote:
> Heh, I'm really confused.  If I do the same thing, I get the following
> result which makes sense as the files would be activated in
> cgroup_apply_cftypes() when the debug controller is registered during
> boot.
> 
>  # mkdir /cgroup2
>  # mount -t cgroup2 cgroup2 /cgroup2
>  # ls /cgroup2/
>  cgroup.controllers      debug.current_css_set
>  cgroup.procs            debug.current_css_set_cg_links
>  cgroup.subtree_control  debug.current_css_set_refcount
>  cgroup.threads          debug.masks
>  debug.csses             debug.taskcount
>  debug.css_links

Never mind.  I can repro after getting rid of some automatic umounts
from rc.local.  Will find out what went wrong.

Thanks!

-- 
tejun

[PATCH cgroup/for-4.13-fixes] cgroup: create dfl_root files on subsys registration

[Tejun Heo]

On subsystem registration, css_populate_dir() is not called on the new
root css, so the interface files for the subsystem on cgrp_dfl_root
aren't created on registration.  This is a residue from the days when
cgrp_dfl_root was used only as the parking spot for unused subsystems,
which no longer is true as it's used as the root for cgroup2.

This is often fine as later operations tend to create them as a part
of mount (cgroup1) or subtree_control operations (cgroup2); however,
it's not difficult to mount cgroup2 with the controller interface
files missing as Waiman found out.

Fix it by invoking css_populate_dir() on the root css on subsys
registration.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Waiman Long <longman@redhat.com>
Cc: stable@vger.kernel.org # v4.5+
---
Hello, Waiman.

Can you please verify that this fixes the bug?

Thanks.

 kernel/cgroup/cgroup.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 620794a..9d33108 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4669,6 +4669,10 @@ int __init cgroup_init(void)
 
 		if (ss->bind)
 			ss->bind(init_css_set.subsys[ssid]);
+
+		mutex_lock(&cgroup_mutex);
+		css_populate_dir(init_css_set.subsys[ssid]);
+		mutex_unlock(&cgroup_mutex);
 	}
 
 	/* init_css_set.subsys[] has been updated, re-hash */

Re: [PATCH cgroup/for-4.13-fixes] cgroup: create dfl_root files on subsys registration

[Waiman Long]

On 07/18/2017 05:57 PM, Tejun Heo wrote:
> On subsystem registration, css_populate_dir() is not called on the new
> root css, so the interface files for the subsystem on cgrp_dfl_root
> aren't created on registration.  This is a residue from the days when
> cgrp_dfl_root was used only as the parking spot for unused subsystems,
> which no longer is true as it's used as the root for cgroup2.
>
> This is often fine as later operations tend to create them as a part
> of mount (cgroup1) or subtree_control operations (cgroup2); however,
> it's not difficult to mount cgroup2 with the controller interface
> files missing as Waiman found out.
>
> Fix it by invoking css_populate_dir() on the root css on subsys
> registration.
>
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Reported-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v4.5+
> ---
> Hello, Waiman.
>
> Can you please verify that this fixes the bug?
>

Yes, this patch fix the problem.

Tested-by:  Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH cgroup/for-4.13-fixes] cgroup: create dfl_root files on subsys registration

[Tejun Heo]

On Tue, Jul 18, 2017 at 06:09:16PM -0400, Waiman Long wrote:
> On 07/18/2017 05:57 PM, Tejun Heo wrote:
> Yes, this patch fix the problem.
> 
> Tested-by:  Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Great, will route the patch through cgroup/for-4.13-fixes.

Thanks for spotting the subtle bug!

-- 
tejun