linux-kernel - Re: [PATCH -last version] freezer_cg: disable writing freezer.state of root cgroup

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]

Message-ID: <4912A73C.4080308@fr.ibm.com>
Date:	Thu, 06 Nov 2008 09:13:48 +0100
From:	Cedric Le Goater <clg@...ibm.com>
To:	Li Zefan <lizf@...fujitsu.com>
CC:	Paul Menage <menage@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Matt Helsley <matthltc@...ibm.com>,
	"Serge E. Hallyn" <serue@...ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux Containers <containers@...ts.linux-foundation.org>
Subject: Re: [PATCH -last version] freezer_cg: disable writing freezer.state
 of root  cgroup

Hello Li ! 

Li Zefan wrote:
> Li Zefan wrote:
>>> Acked-by: Paul Menage <menage@...gle.com>
>>>
>>> I might use the term "non-freezable" rather than "unfreezable".
>>>
>> My bad English :(
>>
>> patch updated. (some annoying leading tabs are also removed in the doc)
>>
> 
> And I forgot to s/EIO/EINVAL in the document..
> 
> This patch should be the last version.. Sorry for the bothering.
> 
> 
> ======================
> 
> From: Li Zefan <lizf@...fujitsu.com>
> Date: Tue, 4 Nov 2008 15:26:29 +0800
> Subject: [PATCH] freezer_cg: disable writing freezer.state of root cgroup
> 
> With this change, control file 'freezer.state' doesn't exist in root
> cgroup, making root cgroup unfreezable.
> 
> I think it's reasonable to disallow freeze tasks in the root cgroup.
> And then we can avoid fork overhead when freezer subsystem is
> compiled but not used.
> 
> Also make writing invalid value to freezer.state returns EINVAL
> rather than EIO. This is more consistent with other cgroup subsystem.
> 
> Signed-off-by: Li Zefan <lizf@...fujitsu.com>
> Acked-by: Paul Menage <menage@...gle.com>

It looks fine 

Acked-by: Cedric Le Goater <clg@...ibm.com>

I've also tested it.

here's a minor comment,

I would make a small macro is_root_freezer() to show explicitly
what we are testing and use the CSS_ROOT bit to test. That's what
the CSS_ROOT bit is for Paul ?

if yes, here's a possible result is below. 

C.

Signed-off-by: Cedric Le Goater <clg@...ibm.com>
---
 kernel/cgroup_freezer.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Index: 2.6.27-lxc/kernel/cgroup_freezer.c
===================================================================
--- 2.6.27-lxc.orig/kernel/cgroup_freezer.c
+++ 2.6.27-lxc/kernel/cgroup_freezer.c
@@ -47,6 +47,11 @@ static inline struct freezer *task_freez
 			    struct freezer, css);
 }
 
+static inline int is_root_freezer(struct freezer *freezer)
+{
+	return test_bit(CSS_ROOT, &freezer->css.flags);
+}
+
 int cgroup_frozen(struct task_struct *task)
 {
 	struct freezer *freezer;
@@ -190,6 +195,13 @@ static void freezer_fork(struct cgroup_s
 	freezer = task_freezer(task);
 	task_unlock(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (is_root_freezer(freezer))
+		return;
+
 	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
@@ -363,6 +375,9 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (is_root_freezer(cgroup_freezer(cgroup)))
+		return 0;
+
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }
--
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/