linux-kernel - Re: [PATCH 1/6] cgroup: add cgroup.isolation_root flag entry to the cgroup filesystem

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:	Thu, 20 Oct 2011 03:12:37 -0700
From:	Paul Menage <paul@...lmenage.org>
To:	Witold Krecicki <wpk@...m.net>
Cc:	Li Zefan <lizf@...fujitsu.com>,
	containers@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] cgroup: add cgroup.isolation_root flag entry to the
 cgroup filesystem

On Fri, Sep 30, 2011 at 4:36 AM, Witold Krecicki <wpk@...m.net> wrote:
> + */
> +static struct cgroup *cgroup_get_isolation_root(struct cgroup *cgrp)
> +{
> +       for (;;) {
> +               if (!cgrp)
> +                       return NULL;
> +               if (isolation_root(cgrp))
> +                       return cgrp;
> +               cgrp = cgrp->parent;
> +       }
> +       return NULL;
> +}

What are the locking requirements for cgroup_get_isolation_root?

> +
> +static int cgroup_isolation_root_write(struct cgroup *cgrp,
> +                                    struct cftype *cft,
> +                                    u64 val)
> +{
> +       if (cgrp->parent == NULL)
> +               return -EBUSY;

EPERM or EINVAL would be more appropriate here, I think.

> +       if (atomic_read(&cgrp->count))
> +               return -EBUSY;

Also need to check for child cgroups, and return -EBUSY?

> +       if (val)
> +               set_bit(CGRP_ISOLATION_ROOT, &cgrp->flags);
> +       else
> +               clear_bit(CGRP_ISOLATION_ROOT, &cgrp->flags);
> +       return 0;
> +}

Arguably we need  to take a lock in these two functions, both to guard
against racing with a creation of a child cgroup or moving in a task
while trying to set its isolation root flag, and to synchronize
readers and writers of the flag. But to be honest I think we can say
that this is one of those cases where we can say that the sysadmin is
dumb enough to have races between his container setup code and his
container population code the result is undefined, as long as we don't
actually crash or leak :-)

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/