From: Andy Lutomirski <luto@amacapital.net>
To: Aditya Kali <adityakali@google.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
"Serge E. Hallyn" <serge@hallyn.com>,
Linux API <linux-api@vger.kernel.org>,
Linux Containers <containers@lists.linux-foundation.org>,
Serge Hallyn <serge.hallyn@ubuntu.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Tejun Heo <tj@kernel.org>,
cgroups@vger.kernel.org, Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCHv1 7/8] cgroup: cgroup namespace setns support
Date: Wed, 22 Oct 2014 11:50:08 -0700 [thread overview]
Message-ID: <CALCETrV+rWN-TqgghXYpSq6Noyu1Z7EOBk4_gs42Z92zJXNnZA@mail.gmail.com> (raw)
In-Reply-To: <CAGr1F2EBDCVrXZd7fOdffQ2C0c25T8co4wfxRc8P0Jb18yq2uQ@mail.gmail.com>
On Wed, Oct 22, 2014 at 11:37 AM, Aditya Kali <adityakali@google.com> wrote:
> On Tue, Oct 21, 2014 at 5:58 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Tue, Oct 21, 2014 at 5:46 PM, Aditya Kali <adityakali@google.com> wrote:
>>> On Tue, Oct 21, 2014 at 3:42 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>> On Tue, Oct 21, 2014 at 3:33 PM, Aditya Kali <adityakali@google.com> wrote:
>>>>>>
>>>>>>> And with explicit permission from
>>>>>>> cgroup subsystem (something like cgroup.may_unshare as you had
>>>>>>> suggested previously), we can make sure that unprivileged processes
>>>>>>> cannot pin themselves. Also, maintaining this invariant (your current
>>>>>>> cgroup is always under your cgroupns-root) keeps the code and the
>>>>>>> semantics simple.
>>>>>>
>>>>>> I actually think it makes the semantics more complex. The less policy
>>>>>> you stick in the kernel, the easier it is to understand the impact of
>>>>>> that policy.
>>>>>>
>>>>>
>>>>> My inclination is towards keeping things simpler - both in code as
>>>>> well as in configuration. I agree that cgroupns might seem
>>>>> "less-flexible", but in its current form, it encourages consistent
>>>>> container configuration. If you have a process that needs to move
>>>>> around between cgroups belonging to different containers, then that
>>>>> process should probably not be inside any container's cgroup
>>>>> namespace. Allowing that will just make the cgroup namespace
>>>>> pretty-much meaningless.
>>>>
>>>> The problem with pinning is that preventing it causes problems
>>>> (specifically, either something potentially complex and incompatible
>>>> needs to be added or unprivileged processes will be able to pin
>>>> themselves).
>>>>
>>>> Unless I'm missing something, a normal cgroupns user doesn't actually
>>>> need kernel pinning support to effectively constrain its members'
>>>> cgroups.
>>>>
>>>
>>> So there are 2 scenarios to consider:
>>>
>>> We have 2 containers with cgroups: /container1 and /container2
>>> Assume process P is running under cgroupns-root '/container1'
>>>
>>> (1) process P wants to 'write' to cgroup.procs outside its
>>> cgroupns-root (say to /container2/cgroup.procs)
>>
>> This, at least, doesn't have the problem with unprivileged processes
>> pinning themselves.
>>
>>> (2) An admin process running in init_cgroup_ns (or any parent cgroupns
>>> with cgroupns-root above /container1) wants to write pid of process P
>>> to /container2/cgroup.procs (which lies outside of P's cgroupns-root)
>>>
>>> For (1), I think its ok to reject such a write. This is consistent
>>> with the restriction in cgroup_file_write added in 'Patch 6' of this
>>> set. I believe this should be independent of visibility of the cgroup
>>> hierarchy for P.
>>>
>>> For (2), we may allow the write to succeed if we make sure that the
>>> process doing the write is an admin process (with CAP_SYS_ADMIN in its
>>> userns AND over P's cgroupns->user_ns).
>>
>> Why is its userns relevant?
>>
>> Why not just check whether the target cgroup is in the process doing
>> the write's cgroupns? (NB: you need to check f_cred, here, not
>> current_cred(), but that's orthogonal.) Then the policy becomes: no
>> user of cgroupfs can move any process outside of the cgroupfs's user's
>> cgroupns root.
>>
> Humm .. it doesn't have to be. I think its simpler to not enforce
> artificial permission checks unless there is a security concern (and
> in this case, there doesn't seem to be any). So I will leave the
> capability check out from here.
>
>> I think I'm okay with this.
>>
>>> If this write succeeds, then:
>>> (a) process P's /proc/<pid>/cgroup does not show anything when viewed
>>> by 'self' or any other process in P's cgrgroupns. I would really like
>>> to avoid showing relative paths or paths outside the cgroupns-root
>>
>> The empty string seems just as problematic to me.
>
> Actually, there is no right answer here. Our options are:
> * show relative path
> -- this will break userspace as /proc/<pid>/cgroup does not show
> relative paths today. This is also very ambiguous (is it relative to
> cgroupns-root or relative to /proc/<pid>cgroup file reader's cgroup?).
>
Confused now. If ".." in /proc/pid/group would be ambiguous, then so
would a path relative to cgroupns root, right? Or am I missing
something?
(I'm not saying that ".." is beautiful or that it won't confuse
things. I'm just not sure why it's ambiguous.)
> * show absolute path
> -- this will also wrong as the process won't be able to make sense of
> it unless it has exposure to the global cgroup hierarchy.
> -- worse case is this that the global path also exists under the
> cgroupns-root ... so now the process thinks its in completely wrong
> cgroup
> -- this exposes system
>
> * show only "/"
> -- this is arguably better, but if the process tires to verify that
> its pid is in cgroup.procs of the cgroupns-root, its in for a
> surprise!
>
> In either case, whatever we expose, the userspace won't be able to use
> this path correctly (worse yet, it associates wrong cgroup for that
> path). So I think its best to not print out the line for default
> hierarchy at all. This happens today when cgroupfs is not mounted. I
> am open to other suggestions.
I suppose that ".." is a possible security problem. If I can force
you to see lots of ..s in there, then I might be about to get you to
write outside cgroupfs.
Grr. No great solution here. I suppose that the empty string isn't
so bad. We could also write something obviously invalid like
"(unreachable)". As long as no one actually creates a cgroup called
"(unreachable)", then this could result in errors but not actual
confusion.
>>> * should we then also allow setns() without first entering the
>>> cgroupns-root? setns also checks the same conditions as in (a) plus it
>>> checks that your current cgroup is descendant of target cgroupns-root.
>>> Alternatively we can special-case setns() to own cgroupns so that it
>>> doesn't fail.
>>
>> I think setns should completely ignore the caller's cgroup and should
>> not change it. Userspace can do this.
>>
>
> All above changes more or less means that tasks cannot pin themselves
> by unsharing cgroupns. Do you agree that we don't need that "explicit
> permission from cgroupfs" anymore (via cgroup.may_unshare file or
> other mechanism)?
Yes, I agree.
>
>>> * migration for these processes will be tricky, if not impossible. But
>>> the admin trying to do this probably doesn't care about it or will
>>> provision for it.
>>
>> Migration for processes in a mntns that have a current directory
>> outside their mntns is also difficult or impossible. Same with
>> pidnses with an fd pointing at /proc/self from an outside-the-pid-ns
>> procfs. Nothing new here.
>>
>> --Andy
>
> Thanks for the review!
No problem.
next prev parent reply other threads:[~2014-10-22 18:50 UTC|newest]
Thread overview: 157+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <adityakali-cgroupns>
2014-07-17 19:52 ` [PATCH 0/5] RFC: CGroup Namespaces Aditya Kali
2014-07-17 19:52 ` [PATCH 1/5] kernfs: Add API to get generate relative kernfs path Aditya Kali
2014-07-24 15:10 ` Serge Hallyn
2014-07-17 19:52 ` [PATCH 2/5] sched: new clone flag CLONE_NEWCGROUP for cgroup namespace Aditya Kali
2014-07-24 17:01 ` Serge Hallyn
2014-07-31 19:48 ` Aditya Kali
2014-08-04 23:12 ` Serge Hallyn
2014-07-17 19:52 ` [PATCH 3/5] cgroup: add function to get task's cgroup on default hierarchy Aditya Kali
2014-07-24 16:59 ` Serge Hallyn
2014-07-17 19:52 ` [PATCH 4/5] cgroup: export cgroup_get() and cgroup_put() Aditya Kali
2014-07-24 17:03 ` Serge Hallyn
2014-07-17 19:52 ` [PATCH 5/5] cgroup: introduce cgroup namespaces Aditya Kali
2014-07-17 19:57 ` Andy Lutomirski
2014-07-17 20:55 ` Aditya Kali
2014-07-18 16:51 ` Andy Lutomirski
2014-07-18 18:51 ` Aditya Kali
2014-07-18 18:57 ` Andy Lutomirski
2014-07-21 22:11 ` Aditya Kali
2014-07-21 22:16 ` Andy Lutomirski
2014-07-23 19:52 ` Aditya Kali
2014-07-18 16:00 ` [PATCH 0/5] RFC: CGroup Namespaces Serge Hallyn
2014-07-24 16:10 ` Serge Hallyn
2014-07-24 16:36 ` Serge Hallyn
2014-07-25 19:29 ` Aditya Kali
2014-07-25 20:27 ` Andy Lutomirski
2014-07-29 4:51 ` Serge E. Hallyn
2014-07-29 15:08 ` Andy Lutomirski
2014-07-29 16:06 ` Serge E. Hallyn
2014-10-13 21:23 ` [PATCHv1 0/8] " Aditya Kali
2014-10-13 21:23 ` [PATCHv1 1/8] kernfs: Add API to generate relative kernfs path Aditya Kali
2014-10-16 16:07 ` Serge E. Hallyn
2014-10-13 21:23 ` [PATCHv1 2/8] sched: new clone flag CLONE_NEWCGROUP for cgroup namespace Aditya Kali
2014-10-16 16:08 ` Serge E. Hallyn
2014-10-13 21:23 ` [PATCHv1 3/8] cgroup: add function to get task's cgroup on default hierarchy Aditya Kali
2014-10-16 16:13 ` Serge E. Hallyn
2014-10-13 21:23 ` [PATCHv1 4/8] cgroup: export cgroup_get() and cgroup_put() Aditya Kali
2014-10-16 16:14 ` Serge E. Hallyn
2014-10-13 21:23 ` [PATCHv1 5/8] cgroup: introduce cgroup namespaces Aditya Kali
2014-10-16 16:37 ` Serge E. Hallyn
2014-10-24 1:03 ` Aditya Kali
2014-10-25 3:16 ` Serge E. Hallyn
2014-10-13 21:23 ` [PATCHv1 6/8] cgroup: restrict cgroup operations within task's cgroupns Aditya Kali
2014-10-17 9:28 ` Serge E. Hallyn
2014-10-22 19:06 ` Aditya Kali
2014-10-19 4:57 ` Eric W. Biederman
2014-10-13 21:23 ` [PATCHv1 7/8] cgroup: cgroup namespace setns support Aditya Kali
2014-10-16 21:12 ` Serge E. Hallyn
2014-10-16 21:17 ` Andy Lutomirski
2014-10-16 21:22 ` Aditya Kali
2014-10-16 21:47 ` Serge E. Hallyn
2014-10-19 5:23 ` Eric W. Biederman
2014-10-19 18:26 ` Andy Lutomirski
2014-10-20 4:55 ` Eric W.Biederman
2014-10-21 0:20 ` Andy Lutomirski
2014-10-21 4:49 ` Eric W. Biederman
2014-10-21 5:03 ` Andy Lutomirski
2014-10-21 5:42 ` Eric W. Biederman
2014-10-21 5:49 ` Andy Lutomirski
2014-10-21 18:49 ` Aditya Kali
2014-10-21 19:02 ` Andy Lutomirski
2014-10-21 22:33 ` Aditya Kali
2014-10-21 22:42 ` Andy Lutomirski
2014-10-22 0:46 ` Aditya Kali
2014-10-22 0:58 ` Andy Lutomirski
2014-10-22 18:37 ` Aditya Kali
2014-10-22 18:50 ` Andy Lutomirski [this message]
2014-10-22 19:42 ` Tejun Heo
2014-10-17 9:52 ` Serge E. Hallyn
2014-10-13 21:23 ` [PATCHv1 8/8] cgroup: mount cgroupns-root when inside non-init cgroupns Aditya Kali
2014-10-17 12:19 ` Serge E. Hallyn
2014-10-14 22:42 ` [PATCHv1 0/8] CGroup Namespaces Andy Lutomirski
2014-10-14 23:33 ` Aditya Kali
2014-10-19 4:54 ` Eric W. Biederman
2015-07-22 18:10 ` Vincent Batts
2014-10-31 19:18 ` [PATCHv2 0/7] " Aditya Kali
2014-10-31 19:18 ` [PATCHv2 1/7] kernfs: Add API to generate relative kernfs path Aditya Kali
2014-10-31 19:18 ` [PATCHv2 2/7] sched: new clone flag CLONE_NEWCGROUP for cgroup namespace Aditya Kali
2014-10-31 19:18 ` [PATCHv2 3/7] cgroup: add function to get task's cgroup on default hierarchy Aditya Kali
2014-10-31 19:18 ` [PATCHv2 4/7] cgroup: export cgroup_get() and cgroup_put() Aditya Kali
2014-10-31 19:18 ` [PATCHv2 5/7] cgroup: introduce cgroup namespaces Aditya Kali
2014-11-01 0:02 ` Andy Lutomirski
2014-11-01 0:58 ` Eric W. Biederman
2014-11-03 23:42 ` Aditya Kali
2014-11-03 23:40 ` Aditya Kali
2014-11-04 1:56 ` Aditya Kali
2014-10-31 19:19 ` [PATCHv2 6/7] cgroup: cgroup namespace setns support Aditya Kali
2014-10-31 19:19 ` [PATCHv2 7/7] cgroup: mount cgroupns-root when inside non-init cgroupns Aditya Kali
2014-11-01 0:07 ` Andy Lutomirski
2014-11-01 2:59 ` Eric W. Biederman
2014-11-01 3:29 ` Andy Lutomirski
2014-11-03 23:12 ` Aditya Kali
2014-11-03 23:15 ` Andy Lutomirski
2014-11-03 23:23 ` Aditya Kali
2014-11-03 23:48 ` Andy Lutomirski
2014-11-04 0:12 ` Aditya Kali
2014-11-04 0:17 ` Andy Lutomirski
2014-11-04 0:49 ` Aditya Kali
2014-11-04 13:57 ` Tejun Heo
2014-11-06 17:28 ` Aditya Kali
2014-11-01 1:09 ` Eric W. Biederman
2014-11-03 22:46 ` Aditya Kali
[not found] ` <CAGr1F2Hd_PS_AscBGMXdZC9qkHGRUp-MeQvJksDOQkRBB3RGoA@mail.gmail.com>
2014-11-03 22:56 ` Andy Lutomirski
2014-11-04 13:46 ` Tejun Heo
2014-11-04 15:00 ` Andy Lutomirski
2014-11-04 15:50 ` Serge E. Hallyn
2014-11-12 17:48 ` Aditya Kali
2014-11-04 1:59 ` Aditya Kali
2014-11-04 13:10 ` [PATCHv2 0/7] CGroup Namespaces Vivek Goyal
2014-11-06 17:33 ` Aditya Kali
2014-11-26 22:58 ` Richard Weinberger
2014-12-02 19:14 ` Aditya Kali
2014-12-05 1:55 ` [PATCHv3 0/8] " Aditya Kali
2014-12-05 1:55 ` [PATCHv3 1/8] kernfs: Add API to generate relative kernfs path Aditya Kali
2014-12-05 1:55 ` [PATCHv3 2/8] sched: new clone flag CLONE_NEWCGROUP for cgroup namespace Aditya Kali
2014-12-05 1:55 ` [PATCHv3 3/8] cgroup: add function to get task's cgroup on default hierarchy Aditya Kali
2014-12-05 1:55 ` [PATCHv3 4/8] cgroup: export cgroup_get() and cgroup_put() Aditya Kali
2014-12-05 1:55 ` [PATCHv3 5/8] cgroup: introduce cgroup namespaces Aditya Kali
2014-12-12 8:54 ` Zefan Li
2014-12-05 1:55 ` [PATCHv3 6/8] cgroup: cgroup namespace setns support Aditya Kali
2014-12-05 1:55 ` [PATCHv3 7/8] cgroup: mount cgroupns-root when inside non-init cgroupns Aditya Kali
2014-12-12 8:55 ` Zefan Li
2014-12-05 1:55 ` [PATCHv3 8/8] cgroup: Add documentation for cgroup namespaces Aditya Kali
2014-12-12 8:54 ` Zefan Li
2015-01-05 22:54 ` Aditya Kali
2014-12-14 23:05 ` Richard Weinberger
2015-01-05 22:48 ` Aditya Kali
2015-01-05 22:52 ` Richard Weinberger
2015-01-05 23:53 ` Eric W. Biederman
2015-01-06 0:07 ` Richard Weinberger
2015-01-06 0:10 ` Aditya Kali
2015-01-06 0:17 ` Richard Weinberger
2015-01-06 23:20 ` Aditya Kali
2015-01-06 23:39 ` Richard Weinberger
2015-01-07 9:28 ` Richard Weinberger
2015-01-07 14:45 ` Eric W. Biederman
2015-01-07 19:30 ` Serge E. Hallyn
2015-01-07 22:14 ` Eric W. Biederman
2015-01-07 22:45 ` Tejun Heo
2015-01-07 23:02 ` Eric W. Biederman
2015-01-07 23:06 ` Tejun Heo
2015-01-07 23:09 ` Eric W. Biederman
2015-01-07 23:16 ` Tejun Heo
2015-01-07 23:27 ` Eric W. Biederman
2015-01-07 23:35 ` Tejun Heo
2015-02-11 3:46 ` Serge E. Hallyn
2015-02-11 4:09 ` Tejun Heo
2015-02-11 4:29 ` Serge E. Hallyn
2015-02-11 5:02 ` Eric W. Biederman
2015-02-11 5:17 ` Tejun Heo
2015-02-11 6:29 ` Eric W. Biederman
2015-02-11 14:36 ` Tejun Heo
2015-02-11 16:00 ` Serge E. Hallyn
2015-02-11 16:03 ` Tejun Heo
2015-02-11 16:18 ` Serge E. Hallyn
2015-02-11 5:10 ` Tejun Heo
2015-01-07 18:57 ` Aditya Kali
2014-12-05 3:20 ` [PATCHv3 0/8] CGroup Namespaces Aditya Kali
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALCETrV+rWN-TqgghXYpSq6Noyu1Z7EOBk4_gs42Z92zJXNnZA@mail.gmail.com \
--to=luto@amacapital.net \
--cc=adityakali@google.com \
--cc=cgroups@vger.kernel.org \
--cc=containers@lists.linux-foundation.org \
--cc=ebiederm@xmission.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=serge.hallyn@ubuntu.com \
--cc=serge@hallyn.com \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).