cms-dev
Hardcoded cgroups and box directory
Currently the location of the cgroup mount (e.g. /sys/fs/cgroup) is hardcoded in autoconf.h. In reality every system seems to have its own traditions with regards to the mount point (I've seen /mnt/cgroup and /cgroup too), moreover some decide to not follow the tradition at all.
Constructive suggestion:
- Add a command line option specifying the location of the cgroup mount.
- When this is not available, check /sys/fs/cgroup, /cgroup and /mnt/cgroup for the necessary files. If not found, fail with a descriptive message.
I can implement that, if you consider this approach reasonable. On a sidenote, none of the values currently in autoconf.h should be hardcoded. A configfile/commandline parameter for each of those would be helpful.
Unfortunately, letting users set the path to cgroupfs would probably lead to security issues. Things like that should be changed only by root. This is also the reason why the other values in autoconf.h are compiled-in. Alternatively, they could be read from a config file editable only by root, but I wanted to avoid parsing text files in a security-sensitive context. Now, I am slowly changing my mind, especially as I would like to add support for per-box options like pinning to a specific CPU or NUMA node.
I think isolate should be able to determine that value dynamically by inspecting /proc/mounts (or, equivalently, the output of the mounts command). Do you see any issues with this approach?
Moreover, it's common practice (see the fifth item of General Reccomendations in [1]) to only create cgroups relative to the one the application is executed in, whereas isolate at the moment uses a root-level "box-/proc/cgroups and /proc/self/cgroup) and creating a new cgroup as a children of that.
[1] http://www.freedesktop.org/wiki/Software/systemd/PaxControlGroups/ [2] https://github.com/cms-dev/isolate/blob/master/isolate.c#L777 [3] https://github.com/cms-dev/isolate/blob/master/isolate.c#L675
True, given that the program creates folders with root permissions within cgroupfs, plain command-line options are probably unsafe. An /etc/isolate.conf, though, would be perfectly OK. Most importantly, it would make the app packageable for particular distributions, hence one would be able to apt-get install isolate. Lack of this possibility is a "usability flaw" in itself currently.
(.. and now I somewhy saw the later comment by lerks)
If /proc/cgroups/ is a universal way of figuring out the proper cgroup mount to use then looking the config up there would be obviously the easiest fix at the moment (the config file might still be useful for other needs, though).
Moreover, it's common practice (see the fifth item of General Reccomendations in [1]) [..]
These "common practices" are not so common as it looks. Basically, these are the opinions of systemd maintainers, not necessarily shared by anybody else ;)
It is actually quite unsure in which direction the cgroup infrastructure in the kernel will evolve: some kernel hackers prefer a common cgroup hiearchy for all controllers (which makes many things easier), others advocate orthogonal hierarchies (which makes many things possible :)).
Given this situation, I would prefer to wait for a while before adding any kind of generic handling of cgroups to isolate.
In the meantime, most needs will be hopefully served by a config file, which will be useful for other things anyway (most importantly the ranges of user and group IDs).
It's true that the document I linked to has been published by the systemd developers and, probably, reflects mainly their view of how the cgroup hierarchy should be structured. However, that was just a marginal remark, which didn't change the main proposal I was putting forward: using /proc/mounts to detect the mount points of the cgroup hierarchies isolate is interested in. This API isn't in any way systemd-related: it's the current way the kernel exposes that information, as documented in this guide (which is part of Linux):
https://www.kernel.org/doc/Documentation/cgroups/cgroups.txt
For the sake of keeping these discussions separate, I'll open a new thread to talk about systemd-caused issues.
I can see why, not knowing the direction cgroup management in the kernel is headed to, you prefer waiting for a while before adding support for anything that is still in development. But I cannot see why in the meantime you don't want to implement a proper solution based on the current stable API, and prefer adding a config file instead, which in my opinion adds more troubles than it solves (e.g. someone, either the packager or the user, has to write or adapt the config file for each system isolate is installed on).
In the general case, I would agree with you and happily implement automatic discovery of cgroupfs mounts. However, in a security-sensitive context (like setuid binaries), I usually have a rather high barrier for accepting non-trivial pieces of code.
This is exactly the same reason for having the UID range hard-wired in isolate binary. However, if we want to have isolate packageable, this must change and it is probably not possible to avoid having a config file. But when we have it, I prefer to use it for the cgroup path as well instead of adding another complex piece of code. Especially before the kernel developers make their mind on what is the right behavior :-)
The "config file solution" is simple and reliable enough to be implemented sooner rather than later (and it does not prevent adding autodetection later on). In this sense I agree with gollux, otherwise one might spend months discussing/testing/verifying or simply worrying about releasing "non-trivial pieces of code" with suid-root.