linux-kernel - Re: [PATCH 2/3] cgroups: add inactive subsystems to rootnode.root

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 2 Dec 2008 14:53:37 -0800
From:	Paul Menage <menage@...gle.com>
To:	Li Zefan <lizf@...fujitsu.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux Containers <containers@...ts.linux-foundation.org>
Subject: Re: [PATCH 2/3] cgroups: add inactive subsystems to 
	rootnode.root_list

On Mon, Dec 1, 2008 at 7:24 PM, Li Zefan <lizf@...fujitsu.com> wrote:
> Paul Menage wrote:
>> On Fri, Nov 28, 2008 at 2:02 AM, Li Zefan <lizf@...fujitsu.com> wrote:
>>> Though for an inactive hierarchy, we have subsys->root == &rootnode,
>>> but rootnode's root_list is always empty.
>>
>> I think you mean "rootnode's subsys_list is always empty".
>>
>
> Yes, I mean subsys_list not root_list.
>
>>> The above code assumes rootnode.subsys_list links all inactive
>>> hierarchies.
>>>
>>> Signed-off-by: Li Zefan <lizf@...fujitsu.com>
>>> ---
>>>  kernel/cgroup.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>>> index 33ba756..4e50e97 100644
>>> --- a/kernel/cgroup.c
>>> +++ b/kernel/cgroup.c
>>> @@ -716,7 +716,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
>>>                        BUG_ON(dummytop->subsys[i]->cgroup != dummytop);
>>>                        cgrp->subsys[i] = dummytop->subsys[i];
>>>                        cgrp->subsys[i]->cgroup = cgrp;
>>> -                       list_add(&ss->sibling, &root->subsys_list);
>>> +                       list_move(&ss->sibling, &root->subsys_list);
>>>                        rcu_assign_pointer(ss->root, root);
>>>                        if (ss->bind)
>>>                                ss->bind(ss, cgrp);
>>> @@ -730,7 +730,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
>>>                        dummytop->subsys[i]->cgroup = dummytop;
>>>                        cgrp->subsys[i] = NULL;
>>>                        rcu_assign_pointer(subsys[i]->root, &rootnode);
>>> -                       list_del(&ss->sibling);
>>> +                       list_move(&ss->sibling, &rootnode.subsys_list);
>>>                } else if (bit & final_bits) {
>>>                        /* Subsystem state should already exist */
>>>                        BUG_ON(!cgrp->subsys[i]);
>>> @@ -2522,6 +2522,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss)
>>>        printk(KERN_INFO "Initializing cgroup subsys %s\n", ss->name);
>>>
>>>        /* Create the top cgroup state for this subsystem */
>>> +       list_add(&ss->sibling, &rootnode.root_list);
>>
>> This is wrong. cgroup_subsys.sibling is a member of the list of
>> subsystems attached to a given root, and rootnode.root_list is a
>> member of the list of hierarchies. Did you actually meant
>> &rootnode.subsys_list?
>>
>
> I thought I corrected all the s/root_list/subsys_list/ in patch v2. :(
>
> If this patch makes sense, I'll fix it, otherwise we'll going to ask Andrew to drop it from -mm.

Looking at the existing code, it's not actually broken - but only by accident.

We aim to add a link for each cgroup, which we do by finding the first
subsystem in each hierarchy and adding a link for its cgroup, and then
adding a link for dummytop if there weren't any subsystems in the
inactive hierarchy.

But since rootnode.subsys_list is always empty, we always skip the
inactive hierarchy in the main loop (if it has any subsystems), and
always add the inactive hierarchy afterwards. So everything works out
OK (all hierarchies do get linked), but it's horribly unreadable.

Having the subsys_list be correct for the inactive hierarchy is
probably a good thing, so this patch should go ahead (once the typos
is fixed).

Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/