many: use transient scope for tracking apps and hooks #7825
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch entirely changes how snapd tracks programs for the purpose
of refresh-app-awareness feature. The old method was using cgroup-v1
only behavior that was stealing processes from systemd's point of view.
The new behavior relies on a DBus API call to systemd, to create
a transient scope with a name that allows snapd to derive snap security
tag from. Due to the requirements of the used API the scope name
must be unique. This is enforced by systemd and handled by creating
a scope with the name "snap.${uuid}.${package}.${app}.scope" for apps
and "snap.${uuid}.${package}.hook.${hook}.scope" for hooks. Services
have a .service unit that is giving us equivalent information so they
do not have or need a scope. The UUID is generated by the kernel.
The old approach relied on privileged snap-confine to alter cgroup
configuration. The new approach requires an unprivileged DBus call and
was thus moved to snap-run. The associated C code and apparmor permissions
have been removed.
The new approach does not support systemd used on Ubuntu 14.04.
The feature is not effective there and application tracking fails
silently.
The code has several TODOs that I plan to address as follow-ups,
given that the feature is behind a feature flag anyway:
- add additional spread test for tracking services
- add additional spread test for tracking non-root applications
In addition I plan to make some organization changes so that large parts
of what is in overlord/snapstate/refresh is moved to sandbox/cgroup
instead. I decided to do this as a follow-up as it is easier to handle
with regards to conflicts.
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
zyga
commented
Nov 29, 2019
This was referenced Nov 29, 2019
|
@pedronis yes, totally. I just was focused on more testing and didn't pay attention to pending/draft PRs. |
bboozzoo
reviewed
Nov 29, 2019
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Reused some tests for PidsInFile Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
chipaca
reviewed
Dec 2, 2019
anonymouse64
reviewed
Dec 2, 2019
There was a problem hiding this comment.
I think this is looking good, I want to test it out a bit and understand what the performance impact of doing this is, but before that's done, I have a number of wording/typo suggestions, with the following outstanding questions:
- why do we still test for /run/snapd/cgroup in the spread tests? I thought we abandoned that idea before it was merged?
- a couple more comments about the locking situation would be appreciated
- a comment on securityTagFromCgroupPath about expecting the path to not end in "/" would be good to have
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
anonymouse64
reviewed
Dec 2, 2019
…sh-app-awareness-v2
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
It used to require a clean path but it is more robust and simple to clean internally. This way the function is less brittle. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
…into feature/refresh-app-awareness-v2
…/refresh-app-awareness-v2
…/refresh-app-awareness-v2
The workaround was required because both snap run and snap-confine were writing to cgroups (indirectly via systemd and directly, respectively). With a single writer we can restore the simpler code. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
…wareness-v2 Remove the sleep workaround added in the branch feature/track-launched-apps
The corresponding code is now in sandbox/cgroup. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
…/refresh-app-awareness-v2
|
I've broken out #9057 with the last non-remove, non-noise changes. |
…sh-app-awareness-v2
|
This now contains almost entirely just the removal of the pid cgroup that used to be used for tracking. There's a a few extra changes like |
|
I've broken out the removal to #9119 |
…sh-app-awareness-v2
|
This now contains the removal of the workaround for systemd's "slow" application of the cgroup transition as well as three one-liner changes to tests and "snap run" code. |
|
Workaround removal is now in #9129 |
|
This looks good, I think the description now needs an update to make sure it reflects what is part of this PR but otherwise I think we can merge it. |
…sh-app-awareness-v2
|
This no longer has anything complex that's not merged separately. I'd like to merge it when green. |
pedronis
added
Squash-merge
jdstrand
removed
the
Needs security review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch entirely changes how snapd tracks programs for the purpose
of refresh-app-awareness feature. The old method was using cgroup-v1
only behavior that was stealing processes from systemd's point of view.
The new behavior relies on a DBus API call to systemd, to create
a transient scope with a name that allows snapd to derive snap security
tag from. Due to the requirements of the used API the scope name
must be unique. This is enforced by systemd and handled by creating
a scope with the name "snap.${uuid}.${package}.${app}.scope" for apps
and "snap.${uuid}.${package}.hook.${hook}.scope" for hooks. Services
have a .service unit that is giving us equivalent information so they
do not have or need a scope. The UUID is generated by the kernel.
The old approach relied on privileged snap-confine to alter cgroup
configuration. The new approach requires an unprivileged DBus call and
was thus moved to snap-run. The associated C code and apparmor permissions
have been removed.
The new approach does not support systemd used on Ubuntu 14.04.
The feature is not effective there and application tracking fails
silently.
The code has several TODOs that I plan to address as follow-ups,
given that the feature is behind a feature flag anyway:
Signed-off-by: Zygmunt Krynicki me@zygoon.pl