cgroups: Set: fix freeze, avoid unnecessary freeze from systemd v1

[kolyshkin]

based on (and currently includes) #3081 and #3067. Keeping this a draft before those two are merged.

1. Fix the inability to freeze the container using its Set() method (with r.Freezer set to Frozen). Add a test.
2. Avoid unnecessary freeze/thaw from system v1 driver. Add a test.

Review commit-by-commit. Please see individual commits for details.

1.0 backport: #3093

[kolyshkin]

As suggested in #3065 (comment), this can be further improved to avoid the freeze entirely if we're sure systemd won't set the deny-all device rule.

Looking at
https://github.com/systemd/systemd/blob/dd376574fd62c6fcf446ee500ca85558a71d645e/src/core/cgroup.c#L1130-L1133
it seems that freeze can be skipped if DevicePolicy is auto (which is the default) AND DeviceAllow list is empty.

Problem is, because of SkipDevices use in update.go, we have to query the current/existing properties first, and only skip freezing if all of the following is true:

1. Current properties have DevicePolicy=auto
2. Current properties have empty DeviceAllow list
3. SkipDevices is true.

[odinuge]

Together with #3081 (review), I think this PR is pretty neat and good.

@kolyshkin in #3065 (comment)

So, one way to fix this would be to have a code that tells whether systemd is going to do deny-all on SetUnitProperties, and skip the freeze it it won't.

If we want to do that for container as well, yeah. Although I don't know how often containers are allowed access to all devices?

For the k8s usage for managing a control group, a simple flag is ok though, but that is up to you and the other runc maintainers.

This is a kludge to a kludge and it's dependent upon systemd internals (which may change) but in these circumstances this may be the best way to proceed.

Yup, it becomes quite a mess... 🙃

The best fix is probably to just start using cgroup v2 instead. crun has opted to just skip dealing with systemd mostly, and use a delegated scope for cgroup v2, and that works kinda ok I guess, but its not perfect. Not sure how they deal with cgroup v1 tho.

[kolyshkin]

If we want to do that for container as well, yeah. Although I don't know how often containers are allowed access to all devices?

Container run via docker run --privileged allows access to all devices. I guess podman and some other runtimes have the same or similar thing.

For the k8s usage for managing a control group, a simple flag is ok though, but that is up to you and the other runc maintainers.

Problem is, from the libcontainer/cgroup we do not know if it's a container or a pod cgroup. Even if it's a pod, it can still have some device access rules (kubernetes doesn't currently set any, but this may change), so having something like Pod: true or SkipFreeze: true is a no go.

The only thing why I don't like the current solution (as implemented by libct/cg/sd/v1: Set: avoid unnecessary freeze/thaw commit in this PR) is it does two calls to dbus to find out whether to skip the freeze, but as we don't have/know the current state, this is necessary.

[cyphar]

@kolyshkin

Looking at
https://github.com/systemd/systemd/blob/dd376574fd62c6fcf446ee500ca85558a71d645e/src/core/cgroup.c#L1130-L1133
it seems that freeze can be skipped if DevicePolicy is auto (which is the default) AND DeviceAllow list is empty.

In that case the device rules would be allow-all, wouldn't they?

                if (c->device_allow || policy != CGROUP_DEVICE_POLICY_AUTO)
                        r = cg_set_attribute("devices", path, "devices.deny", "a");
                else
                        r = cg_set_attribute("devices", path, "devices.allow", "a");

If you have no device_allow list, and the device policy is auto then devices.allow gets set to a which will mean that a container could access devices it shouldn't during the race window (which is worse than the -EACCES inconvenience).

EDIT: Ah, your patch only does the skip if we have SkipDevices and the slice has been configured to be allow-all. Is that a common scenario?

[kolyshkin]

EDIT: Ah, your patch only does the skip if we have SkipDevices and the slice has been configured to be allow-all. Is that a common scenario?

@cyphar Yes, at least for kubernetes that uses libcontainter/cgroup to configure pod cgroups. As pod cgroup is a parent for a few containers cgroups, we do unnecessarily freeze all those containers on update, which is definitely not the way to do.

Those pod tests that we're adding recently are trying to emulate those kubernetes usage scenarios.

[kolyshkin]

Addressed @cyphar review comments, here are the changes I've made:

diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go
index 2a567bb8..1a8e1e3c 100644
--- a/libcontainer/cgroups/systemd/v1.go
+++ b/libcontainer/cgroups/systemd/v1.go
@@ -341,7 +341,7 @@ func (m *legacyManager) GetStats() (*cgroups.Stats, error) {
 // (unlike our fs driver, they will happily write deny-all rules to running
 // containers). So we have to freeze the container to avoid the container get
 // an occasional "permission denied" error.
-func (m *legacyManager) freezeBeforeSet(unitName string, r *configs.Resources) (needsFreeze, needsThaw bool, Err error) {
+func (m *legacyManager) freezeBeforeSet(unitName string, r *configs.Resources) (needsFreeze, needsThaw bool, err error) {
        // Special case for SkipDevices, as used by Kubernetes to create pod
        // cgroups with allow-all device policy).
        if r.SkipDevices {
@@ -352,10 +352,13 @@ func (m *legacyManager) freezeBeforeSet(unitName string, r *configs.Resources) (
                // Interestingly, (1) and (2) are the same here because
                // a non-existent unit returns default properties,
                // and settings in (2) are the defaults.
-               devPolicy, err := getUnitProperty(m.dbus, unitName, "DevicePolicy")
-               if err == nil && devPolicy.Value == dbus.MakeVariant("auto") {
-                       devAllow, err := getUnitProperty(m.dbus, unitName, "DeviceAllow")
-                       if err == nil && devAllow.Value == dbus.MakeVariant([]deviceAllowEntry{}) {
+               //
+               // Do not return errors from getUnitProperty, as they alone
+               // should not prevent Set from working.
+               devPolicy, e := getUnitProperty(m.dbus, unitName, "DevicePolicy")
+               if e == nil && devPolicy.Value == dbus.MakeVariant("auto") {
+                       devAllow, e := getUnitProperty(m.dbus, unitName, "DeviceAllow")
+                       if e == nil && devAllow.Value == dbus.MakeVariant([]deviceAllowEntry{}) {
                                needsFreeze = false
                                needsThaw = false
                                return
@@ -367,8 +370,8 @@ func (m *legacyManager) freezeBeforeSet(unitName string, r *configs.Resources) (
        needsThaw = true
 
        // Check the current freezer state.
-       freezerState, Err := m.GetFreezerState()
-       if Err != nil {
+       freezerState, err := m.GetFreezerState()
+       if err != nil {
                return
        }
        if freezerState == configs.Frozen {

[kolyshkin]

CI went south :(

Could not connect to azure.archive.ubuntu.com:80 (52.252.75.106), connection timed out

[cyphar]

LGTM.