-
-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add capabilities, priority, and oom-score-adjustment support on Linux #404
base: master
Are you sure you want to change the base?
Conversation
I also wonder if i should add functionality for setting scheduling and i/o priorities while at it, and perhaps oom-killer score adjustment (they are useful to have especially for services that change uid, as any wrapper will already run with decreased privileges)? i could also do it later, but it's simple enough to do... i guess all 3 of these should also be specific to linux at this point, because the implementation will differ and so will the range? should these also be behind an option like cgroups? or just |
this is what the setting of the 3 values would look like without the parsing (the input values for
|
3679b5c
to
16ab75c
Compare
I implemented everything. What's left now is basically
|
No strong objections/opinions from me, though as usual good documentation will be important. nice/io_nice/oom_adj are probably unnecessary when cgroups are used properly. If they were to be supported, documentation must clearly state the requirements, i.e.
I think that's fine tbh. It might be worth gating Linux-specific features behind something other than Setting for nice should be possible on other OSes, |
well, the main non-portability for that is range checking, as the acceptable range of values may vary depending on systems maybe i could
|
Ah right, yeah.
True, but I don't think we need to be overly fussed about checking it ourselves. Perhaps add a flag to indicate whether the value is set rather than relying on INT_MIN as a sentinel value.
Yep, that sounds reasonable to me. |
ok, i think i should have a cleaned up PR sometime this evening... |
I added documentation, feature flags, and improved the resource management for the capabilities IAB object, which should be a lot cleaner now. It should be reviewable now, though I still need to test it a bunch more. |
Will review this weekend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I actually have a few minor things that I think would be good to change, as noted. Thanks for the PR.
ifeq ($(SUPPORT_CAPABILITIES),1) | ||
ALL_TEST_LDFLAGS+=-lcap | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really the right place for this. If tests need -lcap
then it should be in TEST_LDFLAGS
. That could be set in mconfig.Linux
/mconfig.Linux.sh
, but see comment there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all linking needs -lcap due to the IAB abstraction in the load-service.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can go into both TEST_LDFLAGS
and LDFLAGS
, what I mean is that this should happen in the build configuration (mconfig.* / configure script) though rather than being done ad-hoc in various makefiles.
@@ -38,6 +38,9 @@ TEST_LDFLAGS=$(TEST_LDFLAGS_BASE) $(TEST_CXXFLAGS) | |||
# Features. | |||
|
|||
SUPPORT_CGROUPS=1 | |||
SUPPORT_CAPABILITIES=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this requires a dependency (in libcap), I'd prefer the default here was SUPPORT_CAPABILITIES=0
. It's fine to have it on-by-default in the configure
script since we can check the dependency is available there (but that should be implemented).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may add a dependency, but it's a dependency that's present on virtually any linux system (it's required by things like... udev), imo it makes much more sense to keep it on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library will be present but not necessarily the headers, and people using a straight Makefile-based build are often just trying out Dinit for personal use. I'd like to make sure that this scenario still works as easily as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but those people can easily disable it; i don't think it's right to default to a configuration where capabilities are not supported, the default build should imo be one that is as distros should build it
it's trivial to disable for somebody just trying it (it's also trivial to just install libcap-devel or whatever, in 99% of scenarios it's easily doable, and you already need to install a development toolchain)
ifeq ($(SUPPORT_CAPABILITIES),1) | ||
ALL_LDFLAGS+=-lcap | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should already be LDFLAGS
if it's needed (see review comments elsewhere for further details).
@@ -467,6 +482,9 @@ STRIPOPTS=$STRIPOPTS | |||
# Feature settings | |||
SUPPORT_CGROUPS=$SUPPORT_CGROUPS | |||
USE_INITGROUPS=$USE_INITGROUPS | |||
SUPPORT_CAPABILITIES=$SUPPORT_CAPABILITIES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If SUPPORT_CAPABILITIES
is 1 then -lcap
needs to be added to LDFLAGS
and TEST_LDFLAGS
if they haven't been specified manually. See for example the tests under the heading "# Check whether sanitizers can be used for tests
" (that's only dealing with TEST_LDFLAGS
but the general principle should be the same for LDFLAGS
).
b5009d7
to
91f0d98
Compare
d6970e2
to
04b6af9
Compare
96f50c9
to
bdb249d
Compare
This is not properly tested yet and does not come with documentation. I mostly want to get an opinion on it for now.
This adds 3 things; when dinit is compiled without support, all of them will raise errors:
capabilities
service field. This is an IAB string; it's parsed withcap_iab_from_string
so it can take any value that can parse, e.g.^cap_sys_time,^cap_net_admin
for ambient caps. It supports the+=
operator, which will append to it, delimiting with a comma.secure-bits
service field, which implements securebits flags as a companion to the above.no-new-privs
which will useprctl
to prevent gaining new privileges acrossexecve
(e.g. suid).For now I want to get an opinion on the overall implementation. One thing I am not sure about for example is the IAB string parsing; I currently do it in
load-service.cc
because it must be done after all the concats have been completed. I find it to be a bit out of place there, however, so maybe there is a better way. Or if you have any other clues about anything, etc...Fixes #398