From bd8e070109cd893f4d9a858c97f4b18a6b8d703b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Jun 2021 08:16:01 -0700 Subject: [PATCH] libct/cg/sd: fix "SkipDevices" handling 1. The meaning of SkipDevices is what it is -- do not set any device-related options. 2. Reverts the part of commit 108ee85b8271e1 which skipped the freeze when the SkipDevices is set. Apparently, the freeze is needed on update even if no Device* properties are being set. 3. Add "runc update" to "runc run [device cgroup deny]" test. Fixes: 752e7a824946a Fixes: 108ee85b8271e Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/systemd/common.go | 2 +- libcontainer/cgroups/systemd/v1.go | 27 ++++++++++++-------------- libcontainer/cgroups/systemd/v2.go | 27 ++++++++++++-------------- tests/integration/dev.bats | 14 ++++++++++++- 4 files changed, 38 insertions(+), 32 deletions(-) diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go index 67c612780..c9995bfe6 100644 --- a/libcontainer/cgroups/systemd/common.go +++ b/libcontainer/cgroups/systemd/common.go @@ -178,7 +178,7 @@ func allowAllDevices() []systemdDbus.Property { // corresponding set of systemd properties to configure the devices correctly. func generateDeviceProperties(r *configs.Resources) ([]systemdDbus.Property, error) { if r.SkipDevices { - return allowAllDevices(), nil + return nil, nil } properties := []systemdDbus.Property{ diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index 06526c54c..1f533c817 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -339,27 +339,24 @@ func (m *legacyManager) Set(r *configs.Resources) error { return err } + // Figure out the current freezer state, so we can revert to it after we + // temporarily freeze the container. + targetFreezerState, err := m.GetFreezerState() + if err != nil { + return err + } + if targetFreezerState == configs.Undefined { + targetFreezerState = configs.Thawed + } + // We have to freeze the container while systemd sets the cgroup settings. // The reason for this is that systemd's application of DeviceAllow rules // is done disruptively, resulting in spurrious errors to common devices // (unlike our fs driver, they will happily write deny-all rules to running // containers). So we freeze the container to avoid them hitting the cgroup // error. But if the freezer cgroup isn't supported, we just warn about it. - targetFreezerState := configs.Undefined - if !m.cgroups.SkipDevices { - // Figure out the current freezer state, so we can revert to it after we - // temporarily freeze the container. - targetFreezerState, err = m.GetFreezerState() - if err != nil { - return err - } - if targetFreezerState == configs.Undefined { - targetFreezerState = configs.Thawed - } - - if err := m.Freeze(configs.Frozen); err != nil { - logrus.Infof("freeze container before SetUnitProperties failed: %v", err) - } + if err := m.Freeze(configs.Frozen); err != nil { + logrus.Infof("freeze container before SetUnitProperties failed: %v", err) } if err := setUnitProperties(m.dbus, getUnitName(m.cgroups), properties...); err != nil { diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index cbd3a308e..262aaca85 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -418,27 +418,24 @@ func (m *unifiedManager) Set(r *configs.Resources) error { return err } + // Figure out the current freezer state, so we can revert to it after we + // temporarily freeze the container. + targetFreezerState, err := m.GetFreezerState() + if err != nil { + return err + } + if targetFreezerState == configs.Undefined { + targetFreezerState = configs.Thawed + } + // We have to freeze the container while systemd sets the cgroup settings. // The reason for this is that systemd's application of DeviceAllow rules // is done disruptively, resulting in spurrious errors to common devices // (unlike our fs driver, they will happily write deny-all rules to running // containers). So we freeze the container to avoid them hitting the cgroup // error. But if the freezer cgroup isn't supported, we just warn about it. - targetFreezerState := configs.Undefined - if !m.cgroups.SkipDevices { - // Figure out the current freezer state, so we can revert to it after we - // temporarily freeze the container. - targetFreezerState, err = m.GetFreezerState() - if err != nil { - return err - } - if targetFreezerState == configs.Undefined { - targetFreezerState = configs.Thawed - } - - if err := m.Freeze(configs.Frozen); err != nil { - logrus.Infof("freeze container before SetUnitProperties failed: %v", err) - } + if err := m.Freeze(configs.Frozen); err != nil { + logrus.Infof("freeze container before SetUnitProperties failed: %v", err) } if err := setUnitProperties(m.dbus, getUnitName(m.cgroups), properties...); err != nil { diff --git a/tests/integration/dev.bats b/tests/integration/dev.bats index 80ceb0a34..35a9ac065 100644 --- a/tests/integration/dev.bats +++ b/tests/integration/dev.bats @@ -33,7 +33,7 @@ function teardown() { [[ "${lines[0]}" =~ "crw-rw-rw".+"1".+"0".+"0".+"5,".+"2".+"/dev/ptmx" ]] } -@test "runc run [device cgroup deny]" { +@test "runc run/update [device cgroup deny]" { requires root update_config ' .linux.resources.devices = [{"allow": false, "access": "rwm"}] @@ -52,6 +52,18 @@ function teardown() { runc exec test_deny sh -c 'head -n 1 /dev/kmsg' [ "$status" -eq 1 ] [[ "${output}" == *'Operation not permitted'* ]] + + runc update test_deny --pids-limit 42 + + # test write + runc exec test_deny sh -c 'hostname | tee /dev/kmsg' + [ "$status" -eq 1 ] + [[ "${output}" == *'Operation not permitted'* ]] + + # test read + runc exec test_deny sh -c 'head -n 1 /dev/kmsg' + [ "$status" -eq 1 ] + [[ "${output}" == *'Operation not permitted'* ]] } @test "runc run [device cgroup allow rw char device]" {