Re: [PATCH 1/6] cgroup: add cgroup.isolation_root flag entry to thecgroup filesystem

[Paul Menage]

On Fri, Sep 30, 2011 at 4:36 AM, Witold Krecicki <wpk@xxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/