This repository was archived by the owner on Nov 17, 2024. It is now read-only.
Repository navigation
This repository was archived by the owner on Nov 17, 2024. It is now read-only.
Hardcoded cgroups and box directory #13
Description
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.
Activity
gollux commentedon Jul 26, 2015
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.
lw commentedon Jul 27, 2015
I think isolate should be able to determine that value dynamically by inspecting
/proc/mounts(or, equivalently, the output of themountscommand). Do you see any issues with this approach?lw commentedon Jul 27, 2015
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-" scheme (see [2] and [3]). I think this should also be fixed, by dynamically looking for the cgroup "path" isolate runs in (by inspecing
/proc/cgroupsand/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
konstantint commentedon Jul 27, 2015
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.konstantint commentedon Jul 27, 2015
(.. 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).gollux commentedon Jul 28, 2015
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).
lw commentedon Jul 29, 2015
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/mountsto 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).
gollux commentedon Jul 29, 2015
konstantint commentedon Jul 29, 2015
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.