Message-ID: <CAGr1F2GcAema-E2q6PFj=R0Z505iD7JshrMuMdfPTJ95wMiQMA@mail.gmail.com>
Date:	Fri, 25 Jul 2014 12:29:52 -0700
From:	Aditya Kali <adityakali@...gle.com>
To:	Serge Hallyn <serge.hallyn@...ntu.com>
Cc:	Tejun Heo <tj@...nel.org>, Li Zefan <lizefan@...wei.com>,
	cgroups@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Linux API <linux-api@...r.kernel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Linux Containers <containers@...ts.linux-foundation.org>,
	Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH 0/5] RFC: CGroup Namespaces

Thank you for your review. I have tried to respond to both your emails here.

On Thu, Jul 24, 2014 at 9:36 AM, Serge Hallyn <serge.hallyn@...ntu.com> wrote:
> Quoting Aditya Kali (adityakali@...gle.com):
>> Background
>>   Cgroups and Namespaces are used together to create “virtual”
>>   containers that isolates the host environment from the processes
>>   running in container. But since cgroups themselves are not
>>   “virtualized”, the task is always able to see global cgroups view
>>   through cgroupfs mount and via /proc/self/cgroup file.
>>
> Hi,
>
> A few questions/comments:
>
> 1. Based on this description, am I to understand that after doing a
>    cgroupns unshare, 'mount -t cgroup cgroup /mnt' by default will
>    still mount the global root cgroup?  Any plans on "changing" that?

This is suggested in the "Possible Extensions of CGROUPNS" section.
More details below.

>    Will attempts to change settings of a cgroup which is not under
>    our current ns be rejected?  (That should be easy to do given your
>    patch 1/5).  Sorry if it's done in the set, I'm jumping around...
>

Currently, only 'cgroup_attach_task', 'cgroup_mkdir' and
'cgroup_rmdir' of cgroups outside of cgroupns-root are prevented. The
read/write of actual cgroup properties are not prevented. Usual
permission checks continue to apply for those. I was hoping that
should be enough, but see more comments towards the end.

> 2. What would be the reprecussions of allowing cgroupns unshare so
>    long as you have ns_capable(CAP_SYS_ADMIN) to the user_ns which
>    created your current ns cgroup?  It'd be a shame if that wasn't
>    on the roadmap.
>

Its certainly on the roadmap, just that some logistics were not clear
at this time. As pointed out by Andy Lutomirski on [PATCH 5/5] of this
series, if we allow cgroupns creation to ns_capable(CAP_SYS_ADMIN)
processes, we may need some kind of explicit permission from the
cgroup subsystem to allow this. One approach could be an explicit
cgroup.may_unshare setting. Alternatively, the cgroup directory (which
is going to become the cgroupns-root) ownership could also be used
here. i.e., the process is ns_capable(CAP_SYS_ADMIN) && it owns the
cgroup directory. There seems to be already a function that allows
similar thing and might be sufficient:

/**
 * capable_wrt_inode_uidgid - Check nsown_capable and uid and gid mapped
 * @inode: The inode in question
 * @cap: The capability in question
 *
 * Return true if the current task has the given capability targeted at
 * its own user namespace and that the given inode's uid and gid are
 * mapped into the current user namespace.
 */
bool capable_wrt_inode_uidgid(const struct inode *inode, int cap)

What do you think? We can enable this for non-init userns once this is
decided on.


> 3. The un-namespaced view of /proc/self/cgroup from a sibling cgroupns
>    makes me wonder whether it wouldn't be more appropriate to leave
>    /proc/self/cgroup always un-filtered, and use /proc/self/nscgroup
>    (or somesuch) to provide the namespaced view.  /proc/self/nscgroup
>    would simply be empty (or say (invalid) or (unreachable)) from a
>    sibling ns.  That will give criu and admin tools like lxc/docker all
>    they need to do simple cgroup setup.
>

It may work for lxc/docker and new applications that use the new
interface. But its difficult to change numerous existing user
applications and libraries that depend on /proc/self/cgroup. Moreover,
even with the new interface, /proc/self/cgroup will continue to leak
system level cgroup information. And fixing this leak is critical to
make the container migratable.

Its easy to correctly handle the read of /proc/<pid>/cgroup from a
sibling cgroupns. Instead of showing unfiltered view, we could just
not show anything (same behavior when the cgroup hierarchy is not
mounted). Will that be more acceptable? I can make that change in the
next version of this series.


>>   $ cat /proc/self/cgroup
>>   0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/c_job_id1
>>
>>   This exposure of cgroup names to the processes running inside a
>>   container results in some problems:
>>   (1) The container names are typically host-container-management-agent
>>       (systemd, docker/libcontainer, etc.) data and leaking its name (or
>>       leaking the hierarchy) reveals too much information about the host
>>       system.
>>   (2) It makes the container migration across machines (CRIU) more
>>       difficult as the container names need to be unique across the
>>       machines in the migration domain.
>>   (3) It makes it difficult to run container management tools (like
>>       docker/libcontainer, lmctfy, etc.) within virtual containers
>>       without adding dependency on some state/agent present outside the
>>       container.
>>
>>   Note that the feature proposed here is completely different than the
>>   “ns cgroup” feature which existed in the linux kernel until recently.
>>   The ns cgroup also attempted to connect cgroups and namespaces by
>>   creating a new cgroup every time a new namespace was created. It did
>>   not solve any of the above mentioned problems and was later dropped
>>   from the kernel.
>>
>> Introducing CGroup Namespaces
>>   With unified cgroup hierarchy
>>   (Documentation/cgroups/unified-hierarchy.txt), the containers can now
>>   have a much more coherent cgroup view and its easy to associate a
>>   container with a single cgroup. This also allows us to virtualize the
>>   cgroup view for tasks inside the container.
>>
>>   The new CGroup Namespace allows a process to “unshare” its cgroup
>>   hierarchy starting from the cgroup its currently in.
>>   For Ex:
>>   $ cat /proc/self/cgroup
>>   0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/c_job_id1
>>   $ ls -l /proc/self/ns/cgroup
>>   lrwxrwxrwx 1 root root 0 2014-07-15 10:37 /proc/self/ns/cgroup -> cgroup:[4026531835]
>>   $ ~/unshare -c  # calls unshare(CLONE_NEWCGROUP) and exec’s /bin/bash
>>   [ns]$ ls -l /proc/self/ns/cgroup
>>   lrwxrwxrwx 1 root root 0 2014-07-15 10:35 /proc/self/ns/cgroup -> cgroup:[4026532183]
>>   # From within new cgroupns, process sees that its in the root cgroup
>>   [ns]$ cat /proc/self/cgroup
>>   0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/
>>
>>   # From global cgroupns:
>>   $ cat /proc/<pid>/cgroup
>>   0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/c_job_id1
>>
>>   The virtualization of /proc/self/cgroup file combined with restricting
>>   the view of cgroup hierarchy by bind-mounting for the
>>   $CGROUP_MOUNT/batchjobs/c_job_id1/ directory to
>>   $CONTAINER_CHROOT/sys/fs/cgroup/) should provide a completely isolated
>>   cgroup view inside the container.
>>
>>   In its current simplistic form, the cgroup namespaces provide
>>   following behavior:
>>
>>   (1) The “root” cgroup for a cgroup namespace is the cgroup in which
>>       the process calling unshare is running.
>>       For ex. if a process in /batchjobs/c_job_id1 cgroup calls unshare,
>>       cgroup /batchjobs/c_job_id1 becomes the cgroupns-root.
>>       For the init_cgroup_ns, this is the real root (“/”) cgroup
>>       (identified in code as cgrp_dfl_root.cgrp).
>>
>>   (2) The cgroupns-root cgroup does not change even if the namespace
>>       creator process later moves to a different cgroup.
>>       $ ~/unshare -c # unshare cgroupns in some cgroup
>>       [ns]$ cat /proc/self/cgroup
>>       0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/
>>       [ns]$ mkdir sub_cgrp_1
>>       [ns]$ echo 0 > sub_cgrp_1/cgroup.procs
>>       [ns]$ cat /proc/self/cgroup
>>       0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/sub_cgrp_1
>>
>>   (3) Each process gets its CGROUPNS specific view of
>>       /proc/<pid>/cgroup.
>>   (a) Processes running inside the cgroup namespace will be able to see
>>       cgroup paths (in /proc/self/cgroup) only inside their root cgroup
>>       [ns]$ sleep 100000 &  # From within unshared cgroupns
>>       [1] 7353
>>       [ns]$ echo 7353 > sub_cgrp_1/cgroup.procs
>>       [ns]$ cat /proc/7353/cgroup
>>       0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/sub_cgrp_1
>>
>>   (b) From global cgroupns, the real cgroup path will be visible:
>>       $ cat /proc/7353/cgroup
>>       0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/c_job_id1/sub_cgrp_1
>>
>>   (c) From a sibling cgroupns, the real path will be visible:
>>       [ns2]$ cat /proc/7353/cgroup
>>       0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/c_job_id1/sub_cgrp_1
>>       (In correct container setup though, it should not be possible to
>>        access PIDs in another container in the first place. This can be
>>        detected changed if desired.)
>>
>>   (4) Processes inside a cgroupns are not allowed to move out of the
>>       cgroupns-root. This is true even if a privileged process in global
>>       cgroupns tries to move the process out of its cgroupns-root.
>>
>>       # From global cgroupns
>>       $ cat /proc/7353/cgroup
>>       0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/c_job_id1/sub_cgrp_1
>>       # cgroupns-root for 7353 is /batchjobs/c_job_id1
>>       $ echo 7353 > batchjobs/c_job_id2/cgroup.procs
>>       -bash: echo: write error: Operation not permitted
>>
>>   (5) setns() is not supported for cgroup namespace in the initial
>>       version.
>
> This combined with the full-path reporting for peer ns cgroups could make
> for fun antics when attaching to an existing container (since we'd have
> to unshare into a new ns cgroup with the same roto as the container).
> I understand you are implying this will be fixed soon though.
>

I am thinking the setns() will be only allowed if
target_cgrpns->cgroupns_root is_descendant_of
current_cgrpns->cgroupns_root. i.e., you will only be setns to a
cgroup namespace which is rooted deeper in hierarchy than your own (in
addition to checking capable_wrt_inode_uidgid(target_cgrpns_inode)).

In addition to this, we need to decide whether its OK for setns() to
also change the cgroup of the task. Consider following example:

[A] ----> [B] ----> C
    ----> D

[A] and [B] are cgroupns-roots. Now, if a task in Cgroup D (which is
under cgroupns [A]) attempts to setns() to cgroupns [B], then its
cgroup should change from /A/D to /A/B. I am concerned about the
side-effects this might cause. Though otherwise, this is a very useful
feature for containers. One could argue that this is similar to
setns() to a mount-namespace which is pivot_root'd somewhere else (in
which case, the attaching task's root "/" moves implicitly with
setns).

Alternatively, we could only allow setns() if
target_cgrpns->cgroupns_root == current->cgroup . I.e., taking above
example again, if process in Cgroup D wants to setns() to cgroupns
[B], then it will first need to move to Cgroup B, and only then the
setns() will succeed. This makes sure that there is no implicit cgroup
move.

WDYT? I haven't prototyped this yet, but will send out a patch after
this series is accepted.

>>   (6) When some thread from a multi-threaded process unshares its
>>       cgroup-namespace, the new cgroupns gets applied to the entire
>>       process (all the threads). This should be OK since
>>       unified-hierarchy only allows process-level containerization. So
>>       all the threads in the process will have the same cgroup. And both
>>       - changing cgroups and unsharing namespaces - are protected under
>>       threadgroup_lock(task).
>>
>>   (7) The cgroup namespace is alive as long as there is atleast 1
>>       process inside it. When the last process exits, the cgroup
>>       namespace is destroyed. The cgroupns-root and the actual cgroups
>>       remain though.
>>
>> Implementation
>>   The current patch-set is based on top of Tejun's cgroup tree (for-next
>>   branch). Its fairly non-intrusive and provides above mentioned
>>   features.
>>
>> Possible extensions of CGROUPNS:
>>   (1) The Documentation/cgroups/unified-hierarchy.txt mentions use of
>>       capabilities to restrict cgroups to administrative users. CGroup
>>       namespaces could be of help here. With cgroup namespaces, it might
>>       be possible to delegate administration of sub-cgroups under a
>>       cgroupns-root to the cgroupns owner.
>
> That would be nice.
>
>>   (2) Provide a cgroupns specific cgroupfs mount. i.e., the following
>>       command when ran from inside a cgroupns should only mount the
>>       hierarchy from cgroupns-root cgroup:
>>       $ mount -t cgroup cgroup <cgroup-mountpoint>
>>       # -o __DEVEL__sane_behavior should be implicit
>>
>>       This is similar to how procfs can be mounted for every PIDNS. This
>>       may have some usecases.
>
> Sorry - I see this answers the first part of a question in my previous email.
> However, the question of whether changes to limits in cgroups which are not
> under our cgroup-ns-root are allowed.
>
> Admittedly the current case with cgmanager is the same - in that it depends
> on proper setup of the container - but cgmanager is geared to recommend
> not mounting the cgroups in the container at all (and we can reject such
> mounts in the contaienr altogether with no loss in functionality) whereas
> you are here encouraging such mounts.  Which is fine - so long as you then
> fully address the potential issues.

It will be nice to have this, but frankly, it may add a bit of
complexity in the cgroup/kernfs code (I will have to prototype and
see). Also same behavior can be obtained simply by bind-mounting
cgroupns-root inside the container. So I am currently inclining
towards rejecting such mounts in favor of simplicity.

Regarding disallowing writes to cgroup files outside of your
cgroupns-root, I think it should possible implement it easily. I will
include it in the next revision of this series.

Thanks,
-- 
Aditya
--
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/

Powered by blists - more mailing lists