specify cgroup ownership semantics #1123

frasertweedale · 2021-09-10T10:40:28Z

cgroups v2 supports secure delegation of cgroups. Accordingly,
control over a cgroup (that is, creation of new child cgroups and
movement of processes and threads among the cgroup subtree exposed
to a container) can be safely delegated to a container. Adjusting
the ownership enables real-world use cases like systemd-based
containers fully isolated in user namespaces.

To encourage adoption of this feature, and secure implementation,
define the semantics of cgroup ownership. Changing/setting the
cgroup ownership is only allowed on cgroups v2, and the specific
files whose ownership can be change are mentioned.

In terms of current practice, this is already the behaviour of crun
(which also chown's the memory.oom.group file), and there is a pull
request for runc: opencontainers/runc#3057
(the behaviour is enabled by an annotation).

config-linux.md

cyphar

Seems reasonable but using MUSTs is far too strong IMHO.

config-linux.md

frasertweedale · 2021-09-22T09:33:14Z

@cyphar thanks for your comments. I'll push an update later today, or tomorrow.

config-linux.md

frasertweedale · 2021-09-23T02:12:22Z

PR updated to fix a grammar issue and relax MUST to SHOULD per @cyphar's comments.

kolyshkin · 2021-09-23T02:32:16Z

One question: should we also require cgroupns? It is assumed with cgroup v2, but what it it is not set in config?

I have just checked that by removing cgroup ns from config.json, and the whole host /sys/fs/cgroup is mounted (for both crun and runc). Meaning we definitely need to add cgroupns to the list of conditions.

frasertweedale · 2021-09-23T04:40:55Z

One question: should we also require cgroupns? It is assumed with cgroup v2, but what it it is not set in config?

I have just checked that by removing cgroup ns from config.json, and the whole host /sys/fs/cgroup is mounted (for both crun and runc). Meaning we definitely need to add cgroupns to the list of conditions.

@kolyshkin good point, I agree. Should we further restrict it to a cgroupns without path specified? That is, where the runtime must create a new cgroupns? Otherwise path could be a reference to an arbitrary, pre-existing cgroupns (possibly the "root" host cgroupns).

edit I pushed an update that adds this requirement, with the empty path as I proposed.

frasertweedale · 2021-10-06T05:39:01Z

Are there any further comments about this PR. Any further concerns? What else needs to be done for this PR to be ready to merge?

tianon

Seems sane to me

@cyphar @kolyshkin willing to take another look? 😅🙏

frasertweedale · 2021-10-12T22:28:13Z

weekly nudge...

config-linux.md

kolyshkin

LGTM (with the two commits squashed).

frasertweedale · 2021-10-16T01:23:51Z

@kolyshkin thank you. I squashed the commits.

frasertweedale · 2021-10-19T06:51:06Z

Seeking more reviews, and hopefully approvals, for this enhancement. Ping @crosbymichael, @cyphar, @dqminh, @giuseppe, @hqhq, @mrunalp, @vbatts, @vishh.

ajwock · 2021-10-20T20:46:15Z

config-linux.md

+
+- `cgroup.procs`
+- `cgroup.subtree_control`
+- `cgroup.threads`


I wanted to mention that there is a file named /sys/kernel/cgroup/delegate (since linux 4.15) which contains a list of delegable files for cgroups. This allows further files to be delegated in the future. My fedora 33 machine has the 3 files mentioned above as well as a new one, memory.oom.group in its /sys/kernel/cgroup/delegate file.

Thanks for the comment. Do folks think it makes sense to update runtime-spec to instruct runtimes to read the files to be delegated from /sys/kernel/cgroup/delegate instead (possibly with the current list as fallback)?

I guess, it should be worded like "The runtime SHOULD use the list of files from /sys/kernel/cgroup/delegate. If that file can not be used, the runtime MAY fall back to the following list" and these three files.

ajwock · 2021-10-20T21:49:38Z

Oh, also I'm curious- is there any reason it would be insecure for v2 cgroups to be delegated to a user other than root while running in the host namespace? Requiring a user namespace before you can manage the cgroup hierarchy adds baggage that would be less than ideal for some use cases, I'm sure.

frasertweedale · 2021-10-21T01:54:07Z

Oh, also I'm curious- is there any reason it would be insecure for v2 cgroups to be delegated to a user other than root while running in the host namespace? Requiring a user namespace before you can manage the cgroup hierarchy adds baggage that would be less than ideal for some use cases, I'm sure.

We could chown to process.user.uid in the namespace, instead of uid 0 unconditionally. That is, the user that will actually execute the initial process in the container. Ordinarily that would be uid 0, but it's possible to specify configs that do something different, and even to specify a config where uid 0 is unmapped in the namespace.

Specifying that behaviour would be a relaxation of the cgroup ownership semantics as currently proposed, thus backwards compatible. Is there a use case that compels us to accommodate this scenario right now? Or shall we keep it simple for now and only chown to the namespace's uid 0 (whether or not that is the uid that will execute the process, and failing if uid 0 is unmapped)?

ajwock · 2021-10-21T12:45:59Z

Personally, I have a use case for managing cgroups within a container but I would prefer not to add the overhead of a user namespace if it isn't necessary.

@giuseppe and others do you see any problem with allowing this?

frasertweedale · 2021-10-22T00:07:03Z

Personally, I have a use case for managing cgroups within a container but I would prefer not to add the overhead of a user namespace if it isn't necessary.

@giuseppe and others do you see any problem with allowing this?

The proposed behaviour doesn't require a (new) user namespace - only that the cgroup should be owned by uid 0 in the container's user namespace (which may be the host user namespace). So to admit your use case, we just need to change it to say that the cgroup should be owned by the host uid that maps to the value of process.user.uid in the container user namespace. Does anyone object to this change?

cgroups v2 supports secure delegation of cgroups. Accordingly, control over a cgroup (that is, creation of new child cgroups and movement of processes and threads among the cgroup subtree exposed to a container) can be safely delegated to a container. Adjusting the ownership enables real-world use cases like systemd-based containers fully isolated in user namespaces. To encourage adoption of this feature, and secure implementation, define the semantics of cgroup ownership. Changing/setting the cgroup ownership should only be performed when: - using cgroups v2, and - container will have a new cgroup namespace, and - cgroupfs will be mounted read/write. The specific files whose ownership should be changed are listed. In terms of current practice, this is already the behaviour of crun (which also chown's the memory.oom.group file), and there is a pull request for runc: opencontainers/runc#3057. Signed-off-by: Fraser Tweedale <[email protected]>

frasertweedale · 2021-11-15T03:49:12Z

Weekly bump. Could someone please merge it, or explain what prevents it?

frasertweedale · 2021-11-22T04:51:13Z

Weekly bump (Nov 22). Could someone please merge it, or explain what else needs to be done or is preventing this PR from being merged?

vbatts · 2021-11-23T15:19:41Z

@frasertweedale thank you for the weekly bumps 😬
Looks like the "MUST" that @cyphar commented on was never marked resolved. Perhaps that's why the other maintainers didn't merge? 🤷

LGTM

frasertweedale · 2021-11-23T23:55:05Z

@vbatts thank you so much! Thanks to all for their feedback and insights.

Delegating cgroups to the container enables more complex workloads, including systemd-based workloads. The OCI runtime-spec was recently updated to explicitly admit such delegation, through specification of cgroup ownership semantics: opencontainers/runtime-spec#1123 Pursuant to the updated OCI runtime-spec, change the ownership of the container's cgroup directory and particular files therein, when using cgroups v2 and when the cgroupfs is to be mounted read/write. As a result of this change, systemd workloads can run in isolated user namespaces on OpenShift when the sandbox's cgroupfs is mounted read/write. It might be possible to implement this feature in other cgroup managers, but that work is deferred. Signed-off-by: Fraser Tweedale <[email protected]>

frasertweedale force-pushed the cgroup-ownership branch from eb7cfaa to f284ad3 Compare September 10, 2021 10:42

frasertweedale mentioned this pull request Sep 10, 2021

chown cgroup to process uid in container namespace opencontainers/runc#3057

Merged

AkihiroSuda reviewed Sep 10, 2021

View reviewed changes

config-linux.md Outdated Show resolved Hide resolved

frasertweedale force-pushed the cgroup-ownership branch from f284ad3 to 9075916 Compare September 11, 2021 06:37

frasertweedale force-pushed the cgroup-ownership branch from 9075916 to 45fba32 Compare September 22, 2021 06:20

AkihiroSuda approved these changes Sep 22, 2021

View reviewed changes

cyphar requested changes Sep 22, 2021

View reviewed changes

config-linux.md Outdated Show resolved Hide resolved

config-linux.md Outdated Show resolved Hide resolved

config-linux.md Outdated Show resolved Hide resolved

kolyshkin reviewed Sep 23, 2021

View reviewed changes

config-linux.md Outdated Show resolved Hide resolved

frasertweedale force-pushed the cgroup-ownership branch from 45fba32 to e1d1e87 Compare September 23, 2021 02:10

frasertweedale force-pushed the cgroup-ownership branch from 183dd28 to bb83ccb Compare September 23, 2021 06:19

tianon approved these changes Oct 6, 2021

View reviewed changes

kolyshkin reviewed Oct 16, 2021

View reviewed changes

config-linux.md Show resolved Hide resolved

kolyshkin approved these changes Oct 16, 2021

View reviewed changes

frasertweedale force-pushed the cgroup-ownership branch from bb83ccb to 42c0604 Compare October 16, 2021 01:23

m-yosefpor mentioned this pull request Oct 20, 2021

Allow a CRI-O container's cgroup to be delegated to a user cri-o/cri-o#5228

Closed

ajwock reviewed Oct 20, 2021

View reviewed changes

frasertweedale force-pushed the cgroup-ownership branch from 42c0604 to f4ef391 Compare October 22, 2021 06:44

vbatts merged commit c2389c3 into opencontainers:main Nov 23, 2021

frasertweedale deleted the cgroup-ownership branch November 23, 2021 23:54

frasertweedale mentioned this pull request Dec 7, 2021

[backport] chown cgroup to process uid in container namespace opencontainers/runc#3311

Closed

tianon mentioned this pull request Mar 2, 2022

Cgroup (namespace) Delegation #948

Open

AkihiroSuda mentioned this pull request Jan 24, 2023

Release v1.1.0-rc.1 #1175

Merged

AkihiroSuda added this to the v1.1.0 milestone Feb 1, 2023

AkihiroSuda mentioned this pull request Jun 26, 2023

Release v1.1.0 #1213

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

specify cgroup ownership semantics #1123

specify cgroup ownership semantics #1123

frasertweedale commented Sep 10, 2021

cyphar left a comment

frasertweedale commented Sep 22, 2021

frasertweedale commented Sep 23, 2021

kolyshkin commented Sep 23, 2021

frasertweedale commented Sep 23, 2021 •

edited

Loading

frasertweedale commented Oct 6, 2021

tianon left a comment

frasertweedale commented Oct 12, 2021

kolyshkin left a comment

frasertweedale commented Oct 16, 2021

frasertweedale commented Oct 19, 2021

ajwock Oct 20, 2021 •

edited

Loading

frasertweedale Oct 21, 2021

kolyshkin Oct 22, 2021

ajwock commented Oct 20, 2021 •

edited

Loading

frasertweedale commented Oct 21, 2021 •

edited

Loading

ajwock commented Oct 21, 2021

frasertweedale commented Oct 22, 2021

frasertweedale commented Nov 15, 2021

frasertweedale commented Nov 22, 2021

vbatts commented Nov 23, 2021

frasertweedale commented Nov 23, 2021

specify cgroup ownership semantics #1123

specify cgroup ownership semantics #1123

Conversation

frasertweedale commented Sep 10, 2021

cyphar left a comment

Choose a reason for hiding this comment

frasertweedale commented Sep 22, 2021

frasertweedale commented Sep 23, 2021

kolyshkin commented Sep 23, 2021

frasertweedale commented Sep 23, 2021 • edited Loading

frasertweedale commented Oct 6, 2021

tianon left a comment

Choose a reason for hiding this comment

frasertweedale commented Oct 12, 2021

kolyshkin left a comment

Choose a reason for hiding this comment

frasertweedale commented Oct 16, 2021

frasertweedale commented Oct 19, 2021

ajwock Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

frasertweedale Oct 21, 2021

Choose a reason for hiding this comment

kolyshkin Oct 22, 2021

Choose a reason for hiding this comment

ajwock commented Oct 20, 2021 • edited Loading

frasertweedale commented Oct 21, 2021 • edited Loading

ajwock commented Oct 21, 2021

frasertweedale commented Oct 22, 2021

frasertweedale commented Nov 15, 2021

frasertweedale commented Nov 22, 2021

vbatts commented Nov 23, 2021

frasertweedale commented Nov 23, 2021

frasertweedale commented Sep 23, 2021 •

edited

Loading

ajwock Oct 20, 2021 •

edited

Loading

ajwock commented Oct 20, 2021 •

edited

Loading

frasertweedale commented Oct 21, 2021 •

edited

Loading