linux-kernel - Re: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem

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: <6599ad830804242251w439dd712tc1919b489535c74c@mail.gmail.com>
Date:	Thu, 24 Apr 2008 22:51:57 -0700
From:	"Paul Menage" <menage@...gle.com>
To:	"Matt Helsley" <matthltc@...ibm.com>
Cc:	Linux-Kernel <linux-kernel@...r.kernel.org>,
	"Cedric Le Goater" <clg@...ibm.com>,
	"Oren Laadan" <orenl@...columbia.edu>,
	"Linus Torvalds" <torvalds@...ux-foundation.org>,
	"Pavel Machek" <pavel@....cz>, linux-pm@...ts.linux-foundation.org,
	"Linux Containers" <containers@...ts.linux-foundation.org>
Subject: Re: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem

>+static const char *freezer_state_strs[] = {
>+	"RUNNING\n",
>+	"FREEZING\n" ,
>+	"FROZEN\n"
>+};

I think it might be cleaner to not include the \n characters in this array.

>+static inline int cgroup_frozen(struct task_struct *task)
>+{
>+	struct cgroup *cgroup = task_cgroup(task, freezer_subsys_id);
>+	struct freezer *freezer = cgroup_freezer(cgroup);
>+	enum freezer_state state;
>+
>+	spin_lock(&freezer->lock);
>+	state = freezer->state;
>+	spin_unlock(&freezer->lock);
>+
>+	return (state == STATE_FROZEN);
>+}

You need to be in an RCU critical section or else hold task_lock() in
order to dereference the cgroup returned from task_cgroup()

I'm not sure that you need to take freezer->lock here - you're just
reading a single word.

>+
>+	if (!capable(CAP_SYS_ADMIN))
>+		return ERR_PTR(-EPERM);
>+

Why does everyone keep throwing calls to check CAP_SYS_ADMIN into
their cgroup create callbacks? You have to be root in order to mount a
cgroups hierarchy in the first place, and filesystem permissions will
control who can create new cgroups.

>+static int freezer_can_attach(struct cgroup_subsys *ss,
>+			      struct cgroup *new_cgroup,
>+			      struct task_struct *task)
>+{
>+	struct freezer *freezer = cgroup_freezer(new_cgroup);
>+	int retval = 0;
>+
>+	if (freezer->state == STATE_FROZEN)
>+		retval = -EBUSY;
>+
>+	return retval;
>+}

You should comment here that the call to cgroup_lock() in the
freezer.state write method prevents a write to that file racing
against an attach, and hence the can_attach() result will remain valid
until the attach completes.

>+static ssize_t freezer_write(struct cgroup *cgroup,
>+			     struct cftype *cft,
>+			     struct file *file,
>+			     const char __user *userbuf,
>+			     size_t nbytes, loff_t *unused_ppos)
>+{
>+	char *buffer;
>+	int retval = 0;
>+	enum freezer_state goal_state;
>+
>+	if (nbytes >= PATH_MAX)
>+		return -E2BIG;
>+
>+	/* +1 for nul-terminator */
>+	buffer = kmalloc(nbytes + 1, GFP_KERNEL);
>+	if (buffer == NULL)
>+		return -ENOMEM;

Given that you're copying a string whose maximum valid length is
"FREEZING" you don't really need to use a dynamically-allocated
buffer.

But I really ought to provide a write_string() method that handles
this kind of copying on behalf of cgroup subsystems, the way it
already does for 64-bit ints.

>+	if (strcmp(buffer, "RUNNING") == 0)
>+		goal_state = STATE_RUNNING;
>+	else if (strcmp(buffer, "FROZEN") == 0)
>+		goal_state = STATE_FROZEN;

Would it make sense to compare against the strings you already have in
the array earlier in the file?

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/