linux-kernel - Re: [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]

Date:	Mon, 3 Nov 2008 21:43:52 -0800
From:	Paul Menage <menage@...gle.com>
To:	Matt Helsley <matthltc@...ibm.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>, Li Zefan <lizf@...fujitsu.com>,
	Linux-Kernel <linux-kernel@...r.kernel.org>,
	Linux Containers <containers@...ts.linux-foundation.org>,
	linux-pm@...ts.linux-foundation.org,
	Cedric Le Goater <clg@...ibm.com>,
	"Serge E. Hallyn" <serue@...ibm.com>
Subject: Re: [PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem

On Mon, Aug 11, 2008 at 3:53 PM, Matt Helsley <matthltc@...ibm.com> wrote:
> +}
> +
> +static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
> +{
> +       struct freezer *freezer;
> +
> +       task_lock(task);
> +       freezer = task_freezer(task);
> +       task_unlock(task);
> +
> +       BUG_ON(freezer->state == STATE_FROZEN);
> +       spin_lock_irq(&freezer->lock);
> +       /* Locking avoids race with FREEZING -> RUNNING transitions. */
> +       if (freezer->state == STATE_FREEZING)
> +               freeze_task(task, true);
> +       spin_unlock_irq(&freezer->lock);
> +}

Sorry for such a delayed response to this patch, but I just noticed
(in mainline now)
the change to move the task_lock() to only encompass the
task_freezer() call.

That results in absolutely zero protection from the task_lock() - as
soon as you drop it, then in theory the current task could move cgroup
and the old freezer structure be freed.

Having said that, I think that in this case any locking my be
unnecessary since task isn't on the tasklist yet, so can't be selected
to move cgroups. (Although this does make me wonder whether
cpuset.c:move_member_tasks_to_cpuset() can fail silently if it races
with a fork).

On top of that, for a system that configures in the cgroup freezer
system but doesn't ever use it, every task is in the same freezer
cgroup (the root cgroup) and task_freezer(task)->lock becomes a global
spinlock. I think this would certainly show up on some benchmarks
although I don't know how bad it would be in a practical sense. But it
might be worth considering making using of the cgroup bind() callback
to track whether or not the freezer subsystem is in use, and
short-circuiting this in freezer_fork() without any locking if you're
not active.

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/