diff --git a/libcontainer/cgroups/cgroups.go b/libcontainer/cgroups/cgroups.go index ba2b2266c..b9ba889b7 100644 --- a/libcontainer/cgroups/cgroups.go +++ b/libcontainer/cgroups/cgroups.go @@ -1,9 +1,24 @@ package cgroups import ( + "errors" + "github.com/opencontainers/runc/libcontainer/configs" ) +var ( + // ErrDevicesUnsupported is an error returned when a cgroup manager + // is not configured to set device rules. + ErrDevicesUnsupported = errors.New("cgroup manager is not configured to set device rules") + + // DevicesSetV1 and DevicesSetV2 are functions to set devices for + // cgroup v1 and v2, respectively. Unless libcontainer/cgroups/devices + // package is imported, it is set to nil, so cgroup managers can't + // manage devices. + DevicesSetV1 func(path string, r *configs.Resources) error + DevicesSetV2 func(path string, r *configs.Resources) error +) + type Manager interface { // Apply creates a cgroup, if not yet created, and adds a process // with the specified pid into that cgroup. A special value of -1 diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go b/libcontainer/cgroups/devices/devicefilter.go similarity index 92% rename from libcontainer/cgroups/ebpf/devicefilter/devicefilter.go rename to libcontainer/cgroups/devices/devicefilter.go index 4e69b35bc..c8cfb5ab5 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go +++ b/libcontainer/cgroups/devices/devicefilter.go @@ -1,10 +1,10 @@ -// Package devicefilter contains eBPF device filter program +// Implements creation of eBPF device filter program. // -// The implementation is based on https://github.com/containers/crun/blob/0.10.2/src/libcrun/ebpf.c +// Based on https://github.com/containers/crun/blob/0.10.2/src/libcrun/ebpf.c // // Although ebpf.c is originally licensed under LGPL-3.0-or-later, the author (Giuseppe Scrivano) // agreed to relicense the file in Apache License 2.0: https://github.com/opencontainers/runc/issues/2144#issuecomment-543116397 -package devicefilter +package devices import ( "errors" @@ -13,7 +13,6 @@ import ( "strconv" "github.com/cilium/ebpf/asm" - devicesemulator "github.com/opencontainers/runc/libcontainer/cgroups/devices" "github.com/opencontainers/runc/libcontainer/devices" "golang.org/x/sys/unix" ) @@ -23,14 +22,14 @@ const ( license = "Apache" ) -// DeviceFilter returns eBPF device filter program and its license string -func DeviceFilter(rules []*devices.Rule) (asm.Instructions, string, error) { +// deviceFilter returns eBPF device filter program and its license string. +func deviceFilter(rules []*devices.Rule) (asm.Instructions, string, error) { // Generate the minimum ruleset for the device rules we are given. While we // don't care about minimum transitions in cgroupv2, using the emulator // gives us a guarantee that the behaviour of devices filtering is the same // as cgroupv1, including security hardenings to avoid misconfiguration // (such as punching holes in wildcard rules). - emu := new(devicesemulator.Emulator) + emu := new(emulator) for _, rule := range rules { if err := emu.Apply(*rule); err != nil { return nil, "", err diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go b/libcontainer/cgroups/devices/devicefilter_test.go similarity index 99% rename from libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go rename to libcontainer/cgroups/devices/devicefilter_test.go index 25703be5a..7b6424d33 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go +++ b/libcontainer/cgroups/devices/devicefilter_test.go @@ -1,4 +1,4 @@ -package devicefilter +package devices import ( "strings" @@ -21,7 +21,7 @@ func hash(s, comm string) string { } func testDeviceFilter(t testing.TB, devices []*devices.Rule, expectedStr string) { - insts, _, err := DeviceFilter(devices) + insts, _, err := deviceFilter(devices) if err != nil { t.Fatalf("%s: %v (devices: %+v)", t.Name(), err, devices) } diff --git a/libcontainer/cgroups/devices/devices.go b/libcontainer/cgroups/devices/devices.go new file mode 100644 index 000000000..844d0563b --- /dev/null +++ b/libcontainer/cgroups/devices/devices.go @@ -0,0 +1,16 @@ +// Package devices contains functionality to manage cgroup devices, which +// is exposed indirectly via libcontainer/cgroups managers. +// +// To enable cgroup managers to manage devices, this package must be imported. +package devices + +import ( + "github.com/opencontainers/runc/libcontainer/cgroups" + "github.com/opencontainers/runc/libcontainer/cgroups/systemd" +) + +func init() { + cgroups.DevicesSetV1 = setV1 + cgroups.DevicesSetV2 = setV2 + systemd.GenerateDeviceProps = systemdProperties +} diff --git a/libcontainer/cgroups/devices/devices_emulator.go b/libcontainer/cgroups/devices/devices_emulator.go index 6c61ee4c0..3e1f49f0f 100644 --- a/libcontainer/cgroups/devices/devices_emulator.go +++ b/libcontainer/cgroups/devices/devices_emulator.go @@ -63,16 +63,16 @@ func (r deviceRules) orderedEntries() []deviceRule { return rules } -type Emulator struct { +type emulator struct { defaultAllow bool rules deviceRules } -func (e *Emulator) IsBlacklist() bool { +func (e *emulator) IsBlacklist() bool { return e.defaultAllow } -func (e *Emulator) IsAllowAll() bool { +func (e *emulator) IsAllowAll() bool { return e.IsBlacklist() && len(e.rules) == 0 } @@ -139,7 +139,7 @@ func parseLine(line string) (*deviceRule, error) { return &rule, nil } -func (e *Emulator) addRule(rule deviceRule) error { //nolint:unparam +func (e *emulator) addRule(rule deviceRule) error { //nolint:unparam if e.rules == nil { e.rules = make(map[deviceMeta]devices.Permissions) } @@ -151,7 +151,7 @@ func (e *Emulator) addRule(rule deviceRule) error { //nolint:unparam return nil } -func (e *Emulator) rmRule(rule deviceRule) error { +func (e *emulator) rmRule(rule deviceRule) error { // Give an error if any of the permissions requested to be removed are // present in a partially-matching wildcard rule, because such rules will // be ignored by cgroupv1. @@ -196,11 +196,11 @@ func (e *Emulator) rmRule(rule deviceRule) error { return nil } -func (e *Emulator) allow(rule *deviceRule) error { +func (e *emulator) allow(rule *deviceRule) error { // This cgroup is configured as a black-list. Reset the entire emulator, // and put is into black-list mode. if rule == nil || rule.meta.node == devices.WildcardDevice { - *e = Emulator{ + *e = emulator{ defaultAllow: true, rules: nil, } @@ -216,11 +216,11 @@ func (e *Emulator) allow(rule *deviceRule) error { return err } -func (e *Emulator) deny(rule *deviceRule) error { +func (e *emulator) deny(rule *deviceRule) error { // This cgroup is configured as a white-list. Reset the entire emulator, // and put is into white-list mode. if rule == nil || rule.meta.node == devices.WildcardDevice { - *e = Emulator{ + *e = emulator{ defaultAllow: false, rules: nil, } @@ -236,7 +236,7 @@ func (e *Emulator) deny(rule *deviceRule) error { return err } -func (e *Emulator) Apply(rule devices.Rule) error { +func (e *emulator) Apply(rule devices.Rule) error { if !rule.Type.CanCgroup() { return fmt.Errorf("cannot add rule [%#v] with non-cgroup type %q", rule, rule.Type) } @@ -260,17 +260,17 @@ func (e *Emulator) Apply(rule devices.Rule) error { return e.deny(innerRule) } -// EmulatorFromList takes a reader to a "devices.list"-like source, and returns +// emulatorFromList takes a reader to a "devices.list"-like source, and returns // a new Emulator that represents the state of the devices cgroup. Note that // black-list devices cgroups cannot be fully reconstructed, due to limitations // in the devices cgroup API. Instead, such cgroups are always treated as // "allow all" cgroups. -func EmulatorFromList(list io.Reader) (*Emulator, error) { +func emulatorFromList(list io.Reader) (*emulator, error) { // Normally cgroups are in black-list mode by default, but the way we // figure out the current mode is whether or not devices.list has an // allow-all rule. So we default to a white-list, and the existence of an // "a *:* rwm" entry will tell us otherwise. - e := &Emulator{ + e := &emulator{ defaultAllow: false, } @@ -304,7 +304,7 @@ func EmulatorFromList(list io.Reader) (*Emulator, error) { // This function is the sole reason for all of Emulator -- to allow us // to figure out how to update a containers' cgroups without causing spurious // device errors (if possible). -func (source *Emulator) Transition(target *Emulator) ([]*devices.Rule, error) { +func (source *emulator) Transition(target *emulator) ([]*devices.Rule, error) { //nolint:revive // Ignore receiver-naming warning. var transitionRules []*devices.Rule oldRules := source.rules @@ -373,8 +373,8 @@ func (source *Emulator) Transition(target *Emulator) ([]*devices.Rule, error) { // cgroup to the emulated filter state (note that this is not the same as a // default cgroupv1 cgroup -- which is allow-all). This is effectively just a // wrapper around Transition() with the source emulator being an empty cgroup. -func (e *Emulator) Rules() ([]*devices.Rule, error) { - defaultCgroup := &Emulator{defaultAllow: false} +func (e *emulator) Rules() ([]*devices.Rule, error) { + defaultCgroup := &emulator{defaultAllow: false} return defaultCgroup.Transition(e) } diff --git a/libcontainer/cgroups/devices/devices_emulator_test.go b/libcontainer/cgroups/devices/devices_emulator_test.go index 4dac242cf..51c1f521e 100644 --- a/libcontainer/cgroups/devices/devices_emulator_test.go +++ b/libcontainer/cgroups/devices/devices_emulator_test.go @@ -31,19 +31,19 @@ import ( func TestDeviceEmulatorLoad(t *testing.T) { tests := []struct { name, list string - expected *Emulator + expected *emulator }{ { name: "BlacklistMode", list: `a *:* rwm`, - expected: &Emulator{ + expected: &emulator{ defaultAllow: true, }, }, { name: "WhitelistBasic", list: `c 4:2 rw`, - expected: &Emulator{ + expected: &emulator{ defaultAllow: false, rules: deviceRules{ { @@ -57,7 +57,7 @@ func TestDeviceEmulatorLoad(t *testing.T) { { name: "WhitelistWildcard", list: `b 0:* m`, - expected: &Emulator{ + expected: &emulator{ defaultAllow: false, rules: deviceRules{ { @@ -72,7 +72,7 @@ func TestDeviceEmulatorLoad(t *testing.T) { name: "WhitelistDuplicate", list: `c *:* rwm c 1:1 r`, - expected: &Emulator{ + expected: &emulator{ defaultAllow: false, rules: deviceRules{ { @@ -102,7 +102,7 @@ c 5:0 rwm c 5:2 rwm c 136:* rwm c 10:200 rwm`, - expected: &Emulator{ + expected: &emulator{ defaultAllow: false, rules: deviceRules{ { @@ -205,7 +205,7 @@ c 10:200 rwm`, test := test // capture range variable t.Run(test.name, func(t *testing.T) { list := bytes.NewBufferString(test.list) - emu, err := EmulatorFromList(list) + emu, err := emulatorFromList(list) if err != nil && test.expected != nil { t.Fatalf("unexpected failure when creating emulator: %v", err) } else if err == nil && test.expected == nil { @@ -223,7 +223,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { tests := []struct { name string rule devices.Rule - base, expected *Emulator + base, expected *emulator }{ // Switch between default modes. { @@ -235,7 +235,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rwm"), Allow: !baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -250,7 +250,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("r"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: !baseDefaultAllow, rules: nil, }, @@ -264,11 +264,11 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rwm"), Allow: baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: nil, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: nil, }, @@ -282,7 +282,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rwm"), Allow: baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -297,7 +297,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("r"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: nil, }, @@ -312,7 +312,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rm"), Allow: !baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -327,7 +327,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("r"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -357,7 +357,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rm"), Allow: !baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -367,7 +367,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("rwm"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -393,7 +393,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rm"), Allow: !baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -403,7 +403,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("rm"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -423,7 +423,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rm"), Allow: !baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -438,7 +438,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("rw"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -463,7 +463,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rm"), Allow: !baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -478,7 +478,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("rw"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -503,7 +503,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("r"), Allow: !baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -518,7 +518,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("rw"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -544,7 +544,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rm"), Allow: baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -559,7 +559,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("r"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -579,7 +579,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rw"), Allow: baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -589,7 +589,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("r"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -609,7 +609,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("rw"), Allow: baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -624,7 +624,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("r"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -644,7 +644,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("r"), Allow: baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -659,7 +659,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("r"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -686,7 +686,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("r"), Allow: baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -712,7 +712,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { Permissions: devices.Permissions("r"), Allow: baseDefaultAllow, }, - base: &Emulator{ + base: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -727,7 +727,7 @@ func testDeviceEmulatorApply(t *testing.T, baseDefaultAllow bool) { }: devices.Permissions("r"), }, }, - expected: &Emulator{ + expected: &emulator{ defaultAllow: baseDefaultAllow, rules: deviceRules{ { @@ -768,13 +768,13 @@ func TestDeviceEmulatorBlacklistApply(t *testing.T) { func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { tests := []struct { name string - source, target *Emulator + source, target *emulator expected []*devices.Rule }{ // No-op changes. { name: "Noop", - source: &Emulator{ + source: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -784,7 +784,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }: devices.Permissions("wm"), }, }, - target: &Emulator{ + target: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -800,7 +800,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { // Switching modes. { name: "SwitchToOtherMode", - source: &Emulator{ + source: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -810,7 +810,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }: devices.Permissions("rwm"), }, }, - target: &Emulator{ + target: &emulator{ defaultAllow: !sourceDefaultAllow, rules: deviceRules{ { @@ -842,7 +842,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { // Rule changes. { name: "RuleAddition", - source: &Emulator{ + source: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -852,7 +852,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }: devices.Permissions("rwm"), }, }, - target: &Emulator{ + target: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -879,7 +879,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }, { name: "RuleRemoval", - source: &Emulator{ + source: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -894,7 +894,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }: devices.Permissions("rwm"), }, }, - target: &Emulator{ + target: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -916,7 +916,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }, { name: "RuleMultipleAdditionRemoval", - source: &Emulator{ + source: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -931,7 +931,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }: devices.Permissions("rw"), }, }, - target: &Emulator{ + target: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -954,7 +954,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { // Modifying the access permissions. { name: "RulePartialAddition", - source: &Emulator{ + source: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -964,7 +964,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }: devices.Permissions("r"), }, }, - target: &Emulator{ + target: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -986,7 +986,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }, { name: "RulePartialRemoval", - source: &Emulator{ + source: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -996,7 +996,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }: devices.Permissions("rw"), }, }, - target: &Emulator{ + target: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -1018,7 +1018,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }, { name: "RulePartialBoth", - source: &Emulator{ + source: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { @@ -1028,7 +1028,7 @@ func testDeviceEmulatorTransition(t *testing.T, sourceDefaultAllow bool) { }: devices.Permissions("rw"), }, }, - target: &Emulator{ + target: &emulator{ defaultAllow: sourceDefaultAllow, rules: deviceRules{ { diff --git a/libcontainer/cgroups/ebpf/ebpf_linux.go b/libcontainer/cgroups/devices/ebpf_linux.go similarity index 98% rename from libcontainer/cgroups/ebpf/ebpf_linux.go rename to libcontainer/cgroups/devices/ebpf_linux.go index 104c74a89..499b4677e 100644 --- a/libcontainer/cgroups/ebpf/ebpf_linux.go +++ b/libcontainer/cgroups/devices/ebpf_linux.go @@ -1,4 +1,4 @@ -package ebpf +package devices import ( "errors" @@ -143,12 +143,12 @@ func haveBpfProgReplace() bool { return haveBpfProgReplaceBool } -// LoadAttachCgroupDeviceFilter installs eBPF device filter program to /sys/fs/cgroup/ directory. +// loadAttachCgroupDeviceFilter installs eBPF device filter program to /sys/fs/cgroup/ directory. // // Requires the system to be running in cgroup2 unified-mode with kernel >= 4.15 . // // https://github.com/torvalds/linux/commit/ebc614f687369f9df99828572b1d85a7c2de3d92 -func LoadAttachCgroupDeviceFilter(insts asm.Instructions, license string, dirFd int) (func() error, error) { +func loadAttachCgroupDeviceFilter(insts asm.Instructions, license string, dirFd int) (func() error, error) { // Increase `ulimit -l` limit to avoid BPF_PROG_LOAD error (#2167). // This limit is not inherited into the container. memlockLimit := &unix.Rlimit{ diff --git a/libcontainer/cgroups/devices/systemd.go b/libcontainer/cgroups/devices/systemd.go new file mode 100644 index 000000000..7e3c6fda8 --- /dev/null +++ b/libcontainer/cgroups/devices/systemd.go @@ -0,0 +1,233 @@ +package devices + +import ( + "bufio" + "errors" + "fmt" + "os" + "strings" + + systemdDbus "github.com/coreos/go-systemd/v22/dbus" + "github.com/godbus/dbus/v5" + "github.com/sirupsen/logrus" + + "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runc/libcontainer/devices" +) + +// systemdProperties takes the configured device rules and generates a +// corresponding set of systemd properties to configure the devices correctly. +func systemdProperties(r *configs.Resources) ([]systemdDbus.Property, error) { + if r.SkipDevices { + return nil, nil + } + + properties := []systemdDbus.Property{ + // Always run in the strictest white-list mode. + newProp("DevicePolicy", "strict"), + // Empty the DeviceAllow array before filling it. + newProp("DeviceAllow", []deviceAllowEntry{}), + } + + // Figure out the set of rules. + configEmu := emulator{} + for _, rule := range r.Devices { + if err := configEmu.Apply(*rule); err != nil { + return nil, fmt.Errorf("unable to apply rule for systemd: %w", err) + } + } + // systemd doesn't support blacklists. So we log a warning, and tell + // systemd to act as a deny-all whitelist. This ruleset will be replaced + // with our normal fallback code. This may result in spurious errors, but + // the only other option is to error out here. + if configEmu.IsBlacklist() { + // However, if we're dealing with an allow-all rule then we can do it. + if configEmu.IsAllowAll() { + return allowAllDevices(), nil + } + logrus.Warn("systemd doesn't support blacklist device rules -- applying temporary deny-all rule") + return properties, nil + } + + // Now generate the set of rules we actually need to apply. Unlike the + // normal devices cgroup, in "strict" mode systemd defaults to a deny-all + // whitelist which is the default for devices.Emulator. + finalRules, err := configEmu.Rules() + if err != nil { + return nil, fmt.Errorf("unable to get simplified rules for systemd: %w", err) + } + var deviceAllowList []deviceAllowEntry + for _, rule := range finalRules { + if !rule.Allow { + // Should never happen. + return nil, fmt.Errorf("[internal error] cannot add deny rule to systemd DeviceAllow list: %v", *rule) + } + switch rule.Type { + case devices.BlockDevice, devices.CharDevice: + default: + // Should never happen. + return nil, fmt.Errorf("invalid device type for DeviceAllow: %v", rule.Type) + } + + entry := deviceAllowEntry{ + Perms: string(rule.Permissions), + } + + // systemd has a fairly odd (though understandable) syntax here, and + // because of the OCI configuration format we have to do quite a bit of + // trickery to convert things: + // + // * Concrete rules with non-wildcard major/minor numbers have to use + // /dev/{block,char} paths. This is slightly odd because it means + // that we cannot add whitelist rules for devices that don't exist, + // but there's not too much we can do about that. + // + // However, path globbing is not support for path-based rules so we + // need to handle wildcards in some other manner. + // + // * Wildcard-minor rules have to specify a "device group name" (the + // second column in /proc/devices). + // + // * Wildcard (major and minor) rules can just specify a glob with the + // type ("char-*" or "block-*"). + // + // The only type of rule we can't handle is wildcard-major rules, and + // so we'll give a warning in that case (note that the fallback code + // will insert any rules systemd couldn't handle). What amazing fun. + + if rule.Major == devices.Wildcard { + // "_ *:n _" rules aren't supported by systemd. + if rule.Minor != devices.Wildcard { + logrus.Warnf("systemd doesn't support '*:n' device rules -- temporarily ignoring rule: %v", *rule) + continue + } + + // "_ *:* _" rules just wildcard everything. + prefix, err := groupPrefix(rule.Type) + if err != nil { + return nil, err + } + entry.Path = prefix + "*" + } else if rule.Minor == devices.Wildcard { + // "_ n:* _" rules require a device group from /proc/devices. + group, err := findDeviceGroup(rule.Type, rule.Major) + if err != nil { + return nil, fmt.Errorf("unable to find device '%v/%d': %w", rule.Type, rule.Major, err) + } + if group == "" { + // Couldn't find a group. + logrus.Warnf("could not find device group for '%v/%d' in /proc/devices -- temporarily ignoring rule: %v", rule.Type, rule.Major, *rule) + continue + } + entry.Path = group + } else { + // "_ n:m _" rules are just a path in /dev/{block,char}/. + switch rule.Type { + case devices.BlockDevice: + entry.Path = fmt.Sprintf("/dev/block/%d:%d", rule.Major, rule.Minor) + case devices.CharDevice: + entry.Path = fmt.Sprintf("/dev/char/%d:%d", rule.Major, rule.Minor) + } + } + deviceAllowList = append(deviceAllowList, entry) + } + + properties = append(properties, newProp("DeviceAllow", deviceAllowList)) + return properties, nil +} + +func newProp(name string, units interface{}) systemdDbus.Property { + return systemdDbus.Property{ + Name: name, + Value: dbus.MakeVariant(units), + } +} + +func groupPrefix(ruleType devices.Type) (string, error) { + switch ruleType { + case devices.BlockDevice: + return "block-", nil + case devices.CharDevice: + return "char-", nil + default: + return "", fmt.Errorf("device type %v has no group prefix", ruleType) + } +} + +// findDeviceGroup tries to find the device group name (as listed in +// /proc/devices) with the type prefixed as required for DeviceAllow, for a +// given (type, major) combination. If more than one device group exists, an +// arbitrary one is chosen. +func findDeviceGroup(ruleType devices.Type, ruleMajor int64) (string, error) { + fh, err := os.Open("/proc/devices") + if err != nil { + return "", err + } + defer fh.Close() + + prefix, err := groupPrefix(ruleType) + if err != nil { + return "", err + } + + scanner := bufio.NewScanner(fh) + var currentType devices.Type + for scanner.Scan() { + // We need to strip spaces because the first number is column-aligned. + line := strings.TrimSpace(scanner.Text()) + + // Handle the "header" lines. + switch line { + case "Block devices:": + currentType = devices.BlockDevice + continue + case "Character devices:": + currentType = devices.CharDevice + continue + case "": + continue + } + + // Skip lines unrelated to our type. + if currentType != ruleType { + continue + } + + // Parse out the (major, name). + var ( + currMajor int64 + currName string + ) + if n, err := fmt.Sscanf(line, "%d %s", &currMajor, &currName); err != nil || n != 2 { + if err == nil { + err = errors.New("wrong number of fields") + } + return "", fmt.Errorf("scan /proc/devices line %q: %w", line, err) + } + + if currMajor == ruleMajor { + return prefix + currName, nil + } + } + if err := scanner.Err(); err != nil { + return "", fmt.Errorf("reading /proc/devices: %w", err) + } + // Couldn't find the device group. + return "", nil +} + +// DeviceAllow is the dbus type "a(ss)" which means we need a struct +// to represent it in Go. +type deviceAllowEntry struct { + Path string + Perms string +} + +func allowAllDevices() []systemdDbus.Property { + // Setting mode to auto and removing all DeviceAllow rules + // results in allowing access to all devices. + return []systemdDbus.Property{ + newProp("DevicePolicy", "auto"), + newProp("DeviceAllow", []deviceAllowEntry{}), + } +} diff --git a/libcontainer/cgroups/devices/systemd_test.go b/libcontainer/cgroups/devices/systemd_test.go new file mode 100644 index 000000000..e20b57c5d --- /dev/null +++ b/libcontainer/cgroups/devices/systemd_test.go @@ -0,0 +1,253 @@ +package devices + +import ( + "bytes" + "os" + "os/exec" + "strings" + "testing" + + "github.com/opencontainers/runc/libcontainer/cgroups" + "github.com/opencontainers/runc/libcontainer/cgroups/systemd" + "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runc/libcontainer/devices" +) + +// TestPodSkipDevicesUpdate checks that updating a pod having SkipDevices: true +// does not result in spurious "permission denied" errors in a container +// running under the pod. The test is somewhat similar in nature to the +// @test "update devices [minimal transition rules]" in tests/integration, +// but uses a pod. +func TestPodSkipDevicesUpdate(t *testing.T) { + if !systemd.IsRunningSystemd() { + t.Skip("Test requires systemd.") + } + if os.Geteuid() != 0 { + t.Skip("Test requires root.") + } + + podName := "system-runc_test_pod" + t.Name() + ".slice" + podConfig := &configs.Cgroup{ + Systemd: true, + Parent: "system.slice", + Name: podName, + Resources: &configs.Resources{ + PidsLimit: 42, + Memory: 32 * 1024 * 1024, + SkipDevices: true, + }, + } + // Create "pod" cgroup (a systemd slice to hold containers). + pm := newManager(t, podConfig) + if err := pm.Apply(-1); err != nil { + t.Fatal(err) + } + if err := pm.Set(podConfig.Resources); err != nil { + t.Fatal(err) + } + + containerConfig := &configs.Cgroup{ + Parent: podName, + ScopePrefix: "test", + Name: "PodSkipDevicesUpdate", + Resources: &configs.Resources{ + Devices: []*devices.Rule{ + // Allow access to /dev/null. + { + Type: devices.CharDevice, + Major: 1, + Minor: 3, + Permissions: "rwm", + Allow: true, + }, + }, + }, + } + + // Create a "container" within the "pod" cgroup. + // This is not a real container, just a process in the cgroup. + cmd := exec.Command("bash", "-c", "while true; do echo > /dev/null; done") + cmd.Env = append(os.Environ(), "LANG=C") + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + // Make sure to not leave a zombie. + defer func() { + // These may fail, we don't care. + _ = cmd.Process.Kill() + _ = cmd.Wait() + }() + + // Put the process into a cgroup. + cm := newManager(t, containerConfig) + if err := cm.Apply(cmd.Process.Pid); err != nil { + t.Fatal(err) + } + // Check that we put the "container" into the "pod" cgroup. + if !strings.HasPrefix(cm.Path("devices"), pm.Path("devices")) { + t.Fatalf("expected container cgroup path %q to be under pod cgroup path %q", + cm.Path("devices"), pm.Path("devices")) + } + if err := cm.Set(containerConfig.Resources); err != nil { + t.Fatal(err) + } + + // Now update the pod a few times. + for i := 0; i < 42; i++ { + podConfig.Resources.PidsLimit++ + podConfig.Resources.Memory += 1024 * 1024 + if err := pm.Set(podConfig.Resources); err != nil { + t.Fatal(err) + } + } + // Kill the "container". + if err := cmd.Process.Kill(); err != nil { + t.Fatal(err) + } + + _ = cmd.Wait() + + // "Container" stderr should be empty. + if stderr.Len() != 0 { + t.Fatalf("container stderr not empty: %s", stderr.String()) + } +} + +func testSkipDevices(t *testing.T, skipDevices bool, expected []string) { + if !systemd.IsRunningSystemd() { + t.Skip("Test requires systemd.") + } + if os.Geteuid() != 0 { + t.Skip("Test requires root.") + } + + podConfig := &configs.Cgroup{ + Parent: "system.slice", + Name: "system-runc_test_pods.slice", + Resources: &configs.Resources{ + SkipDevices: skipDevices, + }, + } + // Create "pods" cgroup (a systemd slice to hold containers). + pm := newManager(t, podConfig) + if err := pm.Apply(-1); err != nil { + t.Fatal(err) + } + if err := pm.Set(podConfig.Resources); err != nil { + t.Fatal(err) + } + + config := &configs.Cgroup{ + Parent: "system-runc_test_pods.slice", + ScopePrefix: "test", + Name: "SkipDevices", + Resources: &configs.Resources{ + Devices: []*devices.Rule{ + // Allow access to /dev/full only. + { + Type: devices.CharDevice, + Major: 1, + Minor: 7, + Permissions: "rwm", + Allow: true, + }, + }, + }, + } + + // Create a "container" within the "pods" cgroup. + // This is not a real container, just a process in the cgroup. + cmd := exec.Command("bash", "-c", "read; echo > /dev/full; cat /dev/null; true") + cmd.Env = append(os.Environ(), "LANG=C") + stdinR, stdinW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + cmd.Stdin = stdinR + var stderr bytes.Buffer + cmd.Stderr = &stderr + err = cmd.Start() + stdinR.Close() + defer stdinW.Close() + if err != nil { + t.Fatal(err) + } + // Make sure to not leave a zombie. + defer func() { + // These may fail, we don't care. + _, _ = stdinW.WriteString("hey\n") + _ = cmd.Wait() + }() + + // Put the process into a cgroup. + m := newManager(t, config) + if err := m.Apply(cmd.Process.Pid); err != nil { + t.Fatal(err) + } + // Check that we put the "container" into the "pod" cgroup. + if !strings.HasPrefix(m.Path("devices"), pm.Path("devices")) { + t.Fatalf("expected container cgroup path %q to be under pod cgroup path %q", + m.Path("devices"), pm.Path("devices")) + } + if err := m.Set(config.Resources); err != nil { + // failed to write "c 1:7 rwm": write /sys/fs/cgroup/devices/system.slice/system-runc_test_pods.slice/test-SkipDevices.scope/devices.allow: operation not permitted + if skipDevices == false && strings.HasSuffix(err.Error(), "/devices.allow: operation not permitted") { + // Cgroup v1 devices controller gives EPERM on trying + // to enable devices that are not enabled + // (skipDevices=false) in a parent cgroup. + // If this happens, test is passing. + return + } + t.Fatal(err) + } + + // Check that we can access /dev/full but not /dev/zero. + if _, err := stdinW.WriteString("wow\n"); err != nil { + t.Fatal(err) + } + if err := cmd.Wait(); err != nil { + t.Fatal(err) + } + for _, exp := range expected { + if !strings.Contains(stderr.String(), exp) { + t.Errorf("expected %q, got: %s", exp, stderr.String()) + } + } +} + +func TestSkipDevicesTrue(t *testing.T) { + testSkipDevices(t, true, []string{ + "echo: write error: No space left on device", + "cat: /dev/null: Operation not permitted", + }) +} + +func TestSkipDevicesFalse(t *testing.T) { + // If SkipDevices is not set for the parent slice, access to both + // devices should fail. This is done to assess the test correctness. + // For cgroup v1, we check for m.Set returning EPERM. + // For cgroup v2, we check for the errors below. + testSkipDevices(t, false, []string{ + "/dev/full: Operation not permitted", + "cat: /dev/null: Operation not permitted", + }) +} + +func newManager(t *testing.T, config *configs.Cgroup) (m cgroups.Manager) { + t.Helper() + var err error + + if cgroups.IsCgroup2UnifiedMode() { + m, err = systemd.NewUnifiedManager(config, "") + } else { + m, err = systemd.NewLegacyManager(config, nil) + } + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = m.Destroy() }) + + return m +} diff --git a/libcontainer/cgroups/devices/v1.go b/libcontainer/cgroups/devices/v1.go new file mode 100644 index 000000000..397c00c8d --- /dev/null +++ b/libcontainer/cgroups/devices/v1.go @@ -0,0 +1,84 @@ +package devices + +import ( + "bytes" + "errors" + "reflect" + + "github.com/opencontainers/runc/libcontainer/cgroups" + "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runc/libcontainer/devices" + "github.com/opencontainers/runc/libcontainer/userns" +) + +var testingSkipFinalCheck bool + +func setV1(path string, r *configs.Resources) error { + if userns.RunningInUserNS() || r.SkipDevices { + return nil + } + // Generate two emulators, one for the current state of the cgroup and one + // for the requested state by the user. + current, err := loadEmulator(path) + if err != nil { + return err + } + target, err := buildEmulator(r.Devices) + if err != nil { + return err + } + + // Compute the minimal set of transition rules needed to achieve the + // requested state. + transitionRules, err := current.Transition(target) + if err != nil { + return err + } + for _, rule := range transitionRules { + file := "devices.deny" + if rule.Allow { + file = "devices.allow" + } + if err := cgroups.WriteFile(path, file, rule.CgroupString()); err != nil { + return err + } + } + + // Final safety check -- ensure that the resulting state is what was + // requested. This is only really correct for white-lists, but for + // black-lists we can at least check that the cgroup is in the right mode. + // + // This safety-check is skipped for the unit tests because we cannot + // currently mock devices.list correctly. + if !testingSkipFinalCheck { + currentAfter, err := loadEmulator(path) + if err != nil { + return err + } + if !target.IsBlacklist() && !reflect.DeepEqual(currentAfter, target) { + return errors.New("resulting devices cgroup doesn't precisely match target") + } else if target.IsBlacklist() != currentAfter.IsBlacklist() { + return errors.New("resulting devices cgroup doesn't match target mode") + } + } + return nil +} + +func loadEmulator(path string) (*emulator, error) { + list, err := cgroups.ReadFile(path, "devices.list") + if err != nil { + return nil, err + } + return emulatorFromList(bytes.NewBufferString(list)) +} + +func buildEmulator(rules []*devices.Rule) (*emulator, error) { + // This defaults to a white-list -- which is what we want! + emu := &emulator{} + for _, rule := range rules { + if err := emu.Apply(*rule); err != nil { + return nil, err + } + } + return emu, nil +} diff --git a/libcontainer/cgroups/fs/devices_test.go b/libcontainer/cgroups/devices/v1_test.go similarity index 60% rename from libcontainer/cgroups/fs/devices_test.go rename to libcontainer/cgroups/devices/v1_test.go index bdd196783..1d1c4c377 100644 --- a/libcontainer/cgroups/fs/devices_test.go +++ b/libcontainer/cgroups/devices/v1_test.go @@ -1,21 +1,34 @@ -package fs +package devices import ( + "os" + "path" "testing" + "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/devices" ) -func TestDevicesSetAllow(t *testing.T) { - path := tempDir(t, "devices") +func init() { + testingSkipFinalCheck = true + cgroups.TestMode = true +} - writeFileContents(t, path, map[string]string{ +func TestSetV1Allow(t *testing.T) { + dir := t.TempDir() + + for file, contents := range map[string]string{ "devices.allow": "", "devices.deny": "", "devices.list": "a *:* rwm", - }) + } { + err := os.WriteFile(path.Join(dir, file), []byte(contents), 0o600) + if err != nil { + t.Fatal(err) + } + } r := &configs.Resources{ Devices: []*devices.Rule{ @@ -29,13 +42,12 @@ func TestDevicesSetAllow(t *testing.T) { }, } - d := &DevicesGroup{TestingSkipFinalCheck: true} - if err := d.Set(path, r); err != nil { + if err := setV1(dir, r); err != nil { t.Fatal(err) } // The default deny rule must be written. - value, err := fscommon.GetCgroupParamString(path, "devices.deny") + value, err := fscommon.GetCgroupParamString(dir, "devices.deny") if err != nil { t.Fatal(err) } @@ -44,7 +56,7 @@ func TestDevicesSetAllow(t *testing.T) { } // Permitted rule must be written. - if value, err := fscommon.GetCgroupParamString(path, "devices.allow"); err != nil { + if value, err := fscommon.GetCgroupParamString(dir, "devices.allow"); err != nil { t.Fatal(err) } else if value != "c 1:5 rwm" { t.Errorf("Got the wrong value (%q), set devices.allow failed.", value) diff --git a/libcontainer/cgroups/fs2/devices.go b/libcontainer/cgroups/devices/v2.go similarity index 83% rename from libcontainer/cgroups/fs2/devices.go rename to libcontainer/cgroups/devices/v2.go index 0d2345607..4bcf860b5 100644 --- a/libcontainer/cgroups/fs2/devices.go +++ b/libcontainer/cgroups/devices/v2.go @@ -1,12 +1,10 @@ -package fs2 +package devices import ( "fmt" "golang.org/x/sys/unix" - "github.com/opencontainers/runc/libcontainer/cgroups/ebpf" - "github.com/opencontainers/runc/libcontainer/cgroups/ebpf/devicefilter" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/devices" "github.com/opencontainers/runc/libcontainer/userns" @@ -53,11 +51,11 @@ func canSkipEBPFError(r *configs.Resources) bool { return true } -func setDevices(dirPath string, r *configs.Resources) error { +func setV2(dirPath string, r *configs.Resources) error { if r.SkipDevices { return nil } - insts, license, err := devicefilter.DeviceFilter(r.Devices) + insts, license, err := deviceFilter(r.Devices) if err != nil { return err } @@ -66,7 +64,7 @@ func setDevices(dirPath string, r *configs.Resources) error { return fmt.Errorf("cannot get dir FD for %s", dirPath) } defer unix.Close(dirFD) - if _, err := ebpf.LoadAttachCgroupDeviceFilter(insts, license, dirFD); err != nil { + if _, err := loadAttachCgroupDeviceFilter(insts, license, dirFD); err != nil { if !canSkipEBPFError(r) { return err } diff --git a/libcontainer/cgroups/fs/devices.go b/libcontainer/cgroups/fs/devices.go index 4527a70eb..0bf3d9deb 100644 --- a/libcontainer/cgroups/fs/devices.go +++ b/libcontainer/cgroups/fs/devices.go @@ -1,20 +1,11 @@ package fs import ( - "bytes" - "errors" - "reflect" - "github.com/opencontainers/runc/libcontainer/cgroups" - cgroupdevices "github.com/opencontainers/runc/libcontainer/cgroups/devices" "github.com/opencontainers/runc/libcontainer/configs" - "github.com/opencontainers/runc/libcontainer/devices" - "github.com/opencontainers/runc/libcontainer/userns" ) -type DevicesGroup struct { - TestingSkipFinalCheck bool -} +type DevicesGroup struct{} func (s *DevicesGroup) Name() string { return "devices" @@ -33,75 +24,14 @@ func (s *DevicesGroup) Apply(path string, r *configs.Resources, pid int) error { return apply(path, pid) } -func loadEmulator(path string) (*cgroupdevices.Emulator, error) { - list, err := cgroups.ReadFile(path, "devices.list") - if err != nil { - return nil, err - } - return cgroupdevices.EmulatorFromList(bytes.NewBufferString(list)) -} - -func buildEmulator(rules []*devices.Rule) (*cgroupdevices.Emulator, error) { - // This defaults to a white-list -- which is what we want! - emu := &cgroupdevices.Emulator{} - for _, rule := range rules { - if err := emu.Apply(*rule); err != nil { - return nil, err - } - } - return emu, nil -} - func (s *DevicesGroup) Set(path string, r *configs.Resources) error { - if userns.RunningInUserNS() || r.SkipDevices { - return nil - } - - // Generate two emulators, one for the current state of the cgroup and one - // for the requested state by the user. - current, err := loadEmulator(path) - if err != nil { - return err - } - target, err := buildEmulator(r.Devices) - if err != nil { - return err - } - - // Compute the minimal set of transition rules needed to achieve the - // requested state. - transitionRules, err := current.Transition(target) - if err != nil { - return err - } - for _, rule := range transitionRules { - file := "devices.deny" - if rule.Allow { - file = "devices.allow" - } - if err := cgroups.WriteFile(path, file, rule.CgroupString()); err != nil { - return err + if cgroups.DevicesSetV1 == nil { + if len(r.Devices) == 0 { + return nil } + return cgroups.ErrDevicesUnsupported } - - // Final safety check -- ensure that the resulting state is what was - // requested. This is only really correct for white-lists, but for - // black-lists we can at least check that the cgroup is in the right mode. - // - // This safety-check is skipped for the unit tests because we cannot - // currently mock devices.list correctly. - if !s.TestingSkipFinalCheck { - currentAfter, err := loadEmulator(path) - if err != nil { - return err - } - if !target.IsBlacklist() && !reflect.DeepEqual(currentAfter, target) { - return errors.New("resulting devices cgroup doesn't precisely match target") - } else if target.IsBlacklist() != currentAfter.IsBlacklist() { - return errors.New("resulting devices cgroup doesn't match target mode") - } - } - return nil + return cgroups.DevicesSetV1(path, r) } func (s *DevicesGroup) GetStats(path string, stats *cgroups.Stats) error { diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index fb4fcc7f7..be4dcc341 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -182,7 +182,7 @@ func (m *manager) Set(r *configs.Resources) error { if err := sys.Set(path, r); err != nil { // When rootless is true, errors from the device subsystem // are ignored, as it is really not expected to work. - if m.cgroups.Rootless && sys.Name() == "devices" { + if m.cgroups.Rootless && sys.Name() == "devices" && !errors.Is(err, cgroups.ErrDevicesUnsupported) { continue } // However, errors from other subsystems are not ignored. diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index 492778e31..d5208d778 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -175,8 +175,10 @@ func (m *manager) Set(r *configs.Resources) error { // When rootless is true, errors from the device subsystem are ignored because it is really not expected to work. // However, errors from other subsystems are not ignored. // see @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error" - if err := setDevices(m.dirPath, r); err != nil && !m.config.Rootless { - return err + if err := setDevices(m.dirPath, r); err != nil { + if !m.config.Rootless || errors.Is(err, cgroups.ErrDevicesUnsupported) { + return err + } } // cpuset (since kernel 5.0) if err := setCpuset(m.dirPath, r); err != nil { @@ -201,6 +203,16 @@ func (m *manager) Set(r *configs.Resources) error { return nil } +func setDevices(dirPath string, r *configs.Resources) error { + if cgroups.DevicesSetV2 == nil { + if len(r.Devices) > 0 { + return cgroups.ErrDevicesUnsupported + } + return nil + } + return cgroups.DevicesSetV2(dirPath, r) +} + func (m *manager) setUnified(res map[string]string) error { for k, v := range res { if strings.Contains(k, "/") { diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go index 98ccc5165..0c3562e6b 100644 --- a/libcontainer/cgroups/systemd/common.go +++ b/libcontainer/cgroups/systemd/common.go @@ -1,7 +1,6 @@ package systemd import ( - "bufio" "context" "errors" "fmt" @@ -17,9 +16,8 @@ import ( dbus "github.com/godbus/dbus/v5" "github.com/sirupsen/logrus" - cgroupdevices "github.com/opencontainers/runc/libcontainer/cgroups/devices" + "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" - "github.com/opencontainers/runc/libcontainer/devices" ) const ( @@ -35,6 +33,8 @@ var ( isRunningSystemdOnce sync.Once isRunningSystemd bool + + GenerateDeviceProps func(*configs.Resources) ([]systemdDbus.Property, error) ) // NOTE: This function comes from package github.com/coreos/go-systemd/util @@ -86,216 +86,6 @@ func ExpandSlice(slice string) (string, error) { return path, nil } -func groupPrefix(ruleType devices.Type) (string, error) { - switch ruleType { - case devices.BlockDevice: - return "block-", nil - case devices.CharDevice: - return "char-", nil - default: - return "", fmt.Errorf("device type %v has no group prefix", ruleType) - } -} - -// findDeviceGroup tries to find the device group name (as listed in -// /proc/devices) with the type prefixed as required for DeviceAllow, for a -// given (type, major) combination. If more than one device group exists, an -// arbitrary one is chosen. -func findDeviceGroup(ruleType devices.Type, ruleMajor int64) (string, error) { - fh, err := os.Open("/proc/devices") - if err != nil { - return "", err - } - defer fh.Close() - - prefix, err := groupPrefix(ruleType) - if err != nil { - return "", err - } - - scanner := bufio.NewScanner(fh) - var currentType devices.Type - for scanner.Scan() { - // We need to strip spaces because the first number is column-aligned. - line := strings.TrimSpace(scanner.Text()) - - // Handle the "header" lines. - switch line { - case "Block devices:": - currentType = devices.BlockDevice - continue - case "Character devices:": - currentType = devices.CharDevice - continue - case "": - continue - } - - // Skip lines unrelated to our type. - if currentType != ruleType { - continue - } - - // Parse out the (major, name). - var ( - currMajor int64 - currName string - ) - if n, err := fmt.Sscanf(line, "%d %s", &currMajor, &currName); err != nil || n != 2 { - if err == nil { - err = errors.New("wrong number of fields") - } - return "", fmt.Errorf("scan /proc/devices line %q: %w", line, err) - } - - if currMajor == ruleMajor { - return prefix + currName, nil - } - } - if err := scanner.Err(); err != nil { - return "", fmt.Errorf("reading /proc/devices: %w", err) - } - // Couldn't find the device group. - return "", nil -} - -// DeviceAllow is the dbus type "a(ss)" which means we need a struct -// to represent it in Go. -type deviceAllowEntry struct { - Path string - Perms string -} - -func allowAllDevices() []systemdDbus.Property { - // Setting mode to auto and removing all DeviceAllow rules - // results in allowing access to all devices. - return []systemdDbus.Property{ - newProp("DevicePolicy", "auto"), - newProp("DeviceAllow", []deviceAllowEntry{}), - } -} - -// generateDeviceProperties takes the configured device rules and generates a -// corresponding set of systemd properties to configure the devices correctly. -func generateDeviceProperties(r *configs.Resources) ([]systemdDbus.Property, error) { - if r.SkipDevices { - return nil, nil - } - - properties := []systemdDbus.Property{ - // Always run in the strictest white-list mode. - newProp("DevicePolicy", "strict"), - // Empty the DeviceAllow array before filling it. - newProp("DeviceAllow", []deviceAllowEntry{}), - } - - // Figure out the set of rules. - configEmu := &cgroupdevices.Emulator{} - for _, rule := range r.Devices { - if err := configEmu.Apply(*rule); err != nil { - return nil, fmt.Errorf("unable to apply rule for systemd: %w", err) - } - } - // systemd doesn't support blacklists. So we log a warning, and tell - // systemd to act as a deny-all whitelist. This ruleset will be replaced - // with our normal fallback code. This may result in spurious errors, but - // the only other option is to error out here. - if configEmu.IsBlacklist() { - // However, if we're dealing with an allow-all rule then we can do it. - if configEmu.IsAllowAll() { - return allowAllDevices(), nil - } - logrus.Warn("systemd doesn't support blacklist device rules -- applying temporary deny-all rule") - return properties, nil - } - - // Now generate the set of rules we actually need to apply. Unlike the - // normal devices cgroup, in "strict" mode systemd defaults to a deny-all - // whitelist which is the default for devices.Emulator. - finalRules, err := configEmu.Rules() - if err != nil { - return nil, fmt.Errorf("unable to get simplified rules for systemd: %w", err) - } - var deviceAllowList []deviceAllowEntry - for _, rule := range finalRules { - if !rule.Allow { - // Should never happen. - return nil, fmt.Errorf("[internal error] cannot add deny rule to systemd DeviceAllow list: %v", *rule) - } - switch rule.Type { - case devices.BlockDevice, devices.CharDevice: - default: - // Should never happen. - return nil, fmt.Errorf("invalid device type for DeviceAllow: %v", rule.Type) - } - - entry := deviceAllowEntry{ - Perms: string(rule.Permissions), - } - - // systemd has a fairly odd (though understandable) syntax here, and - // because of the OCI configuration format we have to do quite a bit of - // trickery to convert things: - // - // * Concrete rules with non-wildcard major/minor numbers have to use - // /dev/{block,char} paths. This is slightly odd because it means - // that we cannot add whitelist rules for devices that don't exist, - // but there's not too much we can do about that. - // - // However, path globbing is not support for path-based rules so we - // need to handle wildcards in some other manner. - // - // * Wildcard-minor rules have to specify a "device group name" (the - // second column in /proc/devices). - // - // * Wildcard (major and minor) rules can just specify a glob with the - // type ("char-*" or "block-*"). - // - // The only type of rule we can't handle is wildcard-major rules, and - // so we'll give a warning in that case (note that the fallback code - // will insert any rules systemd couldn't handle). What amazing fun. - - if rule.Major == devices.Wildcard { - // "_ *:n _" rules aren't supported by systemd. - if rule.Minor != devices.Wildcard { - logrus.Warnf("systemd doesn't support '*:n' device rules -- temporarily ignoring rule: %v", *rule) - continue - } - - // "_ *:* _" rules just wildcard everything. - prefix, err := groupPrefix(rule.Type) - if err != nil { - return nil, err - } - entry.Path = prefix + "*" - } else if rule.Minor == devices.Wildcard { - // "_ n:* _" rules require a device group from /proc/devices. - group, err := findDeviceGroup(rule.Type, rule.Major) - if err != nil { - return nil, fmt.Errorf("unable to find device '%v/%d': %w", rule.Type, rule.Major, err) - } - if group == "" { - // Couldn't find a group. - logrus.Warnf("could not find device group for '%v/%d' in /proc/devices -- temporarily ignoring rule: %v", rule.Type, rule.Major, *rule) - continue - } - entry.Path = group - } else { - // "_ n:m _" rules are just a path in /dev/{block,char}/. - switch rule.Type { - case devices.BlockDevice: - entry.Path = fmt.Sprintf("/dev/block/%d:%d", rule.Major, rule.Minor) - case devices.CharDevice: - entry.Path = fmt.Sprintf("/dev/char/%d:%d", rule.Major, rule.Minor) - } - } - deviceAllowList = append(deviceAllowList, entry) - } - - properties = append(properties, newProp("DeviceAllow", deviceAllowList)) - return properties, nil -} - func newProp(name string, units interface{}) systemdDbus.Property { return systemdDbus.Property{ Name: name, @@ -526,3 +316,16 @@ func addCpuset(cm *dbusConnManager, props *[]systemdDbus.Property, cpus, mems st } return nil } + +// generateDeviceProperties takes the configured device rules and generates a +// corresponding set of systemd properties to configure the devices correctly. +func generateDeviceProperties(r *configs.Resources) ([]systemdDbus.Property, error) { + if GenerateDeviceProps == nil { + if len(r.Devices) > 0 { + return nil, cgroups.ErrDevicesUnsupported + } + return nil, nil + } + + return GenerateDeviceProps(r) +} diff --git a/libcontainer/cgroups/systemd/devices.go b/libcontainer/cgroups/systemd/devices.go new file mode 100644 index 000000000..edd1f1585 --- /dev/null +++ b/libcontainer/cgroups/systemd/devices.go @@ -0,0 +1,74 @@ +package systemd + +import ( + "reflect" + + dbus "github.com/godbus/dbus/v5" + + "github.com/opencontainers/runc/libcontainer/configs" +) + +// freezeBeforeSet answers whether there is a need to freeze the cgroup before +// applying its systemd unit properties, and thaw after, while avoiding +// unnecessary freezer state changes. +// +// The reason why we have to freeze is that systemd's application of device +// rules is done disruptively, resulting in spurious errors to common devices +// (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) { + // Special case for SkipDevices, as used by Kubernetes to create pod + // cgroups with allow-all device policy). + if r.SkipDevices { + if r.SkipFreezeOnSet { + // Both needsFreeze and needsThaw are false. + return + } + + // No need to freeze if SkipDevices is set, and either + // (1) systemd unit does not (yet) exist, or + // (2) it has DevicePolicy=auto and empty DeviceAllow list. + // + // Interestingly, (1) and (2) are the same here because + // a non-existent unit returns default properties, + // and settings in (2) are the defaults. + // + // Do not return errors from getUnitTypeProperty, as they alone + // should not prevent Set from working. + + unitType := getUnitType(unitName) + + devPolicy, e := getUnitTypeProperty(m.dbus, unitName, unitType, "DevicePolicy") + if e == nil && devPolicy.Value == dbus.MakeVariant("auto") { + devAllow, e := getUnitTypeProperty(m.dbus, unitName, unitType, "DeviceAllow") + if e == nil { + if rv := reflect.ValueOf(devAllow.Value.Value()); rv.Kind() == reflect.Slice && rv.Len() == 0 { + needsFreeze = false + needsThaw = false + return + } + } + } + } + + needsFreeze = true + needsThaw = true + + // Check the current freezer state. + freezerState, err := m.GetFreezerState() + if err != nil { + return + } + if freezerState == configs.Frozen { + // Already frozen, and should stay frozen. + needsFreeze = false + needsThaw = false + } + + if r.Freezer == configs.Frozen { + // Will be frozen anyway -- no need to thaw. + needsThaw = false + } + return +} diff --git a/libcontainer/cgroups/systemd/v1_test.go b/libcontainer/cgroups/systemd/freeze_test.go similarity index 63% rename from libcontainer/cgroups/systemd/v1_test.go rename to libcontainer/cgroups/systemd/freeze_test.go index 7c7ef55fc..760578654 100644 --- a/libcontainer/cgroups/systemd/v1_test.go +++ b/libcontainer/cgroups/systemd/freeze_test.go @@ -1,15 +1,16 @@ package systemd import ( + "bufio" + "bytes" "os" "os/exec" "strings" "testing" - "golang.org/x/sys/unix" - "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" + "golang.org/x/sys/unix" ) func TestFreezeBeforeSet(t *testing.T) { @@ -218,3 +219,138 @@ func requireV1(t *testing.T) { t.Skip("Test requires root.") } } + +func TestFreezePodCgroup(t *testing.T) { + if !IsRunningSystemd() { + t.Skip("Test requires systemd.") + } + if os.Geteuid() != 0 { + t.Skip("Test requires root.") + } + + podConfig := &configs.Cgroup{ + Parent: "system.slice", + Name: "system-runc_test_pod.slice", + Resources: &configs.Resources{ + SkipDevices: true, + Freezer: configs.Frozen, + }, + } + // Create a "pod" cgroup (a systemd slice to hold containers), + // which is frozen initially. + pm := newManager(t, podConfig) + if err := pm.Apply(-1); err != nil { + t.Fatal(err) + } + + if err := pm.Set(podConfig.Resources); err != nil { + t.Fatal(err) + } + + // Check the pod is frozen. + pf, err := pm.GetFreezerState() + if err != nil { + t.Fatal(err) + } + if pf != configs.Frozen { + t.Fatalf("expected pod to be frozen, got %v", pf) + } + + // Create a "container" within the "pod" cgroup. + // This is not a real container, just a process in the cgroup. + containerConfig := &configs.Cgroup{ + Parent: "system-runc_test_pod.slice", + ScopePrefix: "test", + Name: "inner-container", + Resources: &configs.Resources{}, + } + + cmd := exec.Command("bash", "-c", "while read; do echo $REPLY; done") + cmd.Env = append(os.Environ(), "LANG=C") + + // Setup stdin. + stdinR, stdinW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + cmd.Stdin = stdinR + + // Setup stdout. + stdoutR, stdoutW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + cmd.Stdout = stdoutW + rdr := bufio.NewReader(stdoutR) + + // Setup stderr. + var stderr bytes.Buffer + cmd.Stderr = &stderr + + err = cmd.Start() + stdinR.Close() + stdoutW.Close() + defer func() { + _ = stdinW.Close() + _ = stdoutR.Close() + }() + if err != nil { + t.Fatal(err) + } + // Make sure to not leave a zombie. + defer func() { + // These may fail, we don't care. + _ = cmd.Process.Kill() + _ = cmd.Wait() + }() + + // Put the process into a cgroup. + cm := newManager(t, containerConfig) + + if err := cm.Apply(cmd.Process.Pid); err != nil { + t.Fatal(err) + } + if err := cm.Set(containerConfig.Resources); err != nil { + t.Fatal(err) + } + // Check that we put the "container" into the "pod" cgroup. + if !strings.HasPrefix(cm.Path("freezer"), pm.Path("freezer")) { + t.Fatalf("expected container cgroup path %q to be under pod cgroup path %q", + cm.Path("freezer"), pm.Path("freezer")) + } + // Check the container is not reported as frozen despite the frozen parent. + cf, err := cm.GetFreezerState() + if err != nil { + t.Fatal(err) + } + if cf != configs.Thawed { + t.Fatalf("expected container to be thawed, got %v", cf) + } + + // Unfreeze the pod. + if err := pm.Freeze(configs.Thawed); err != nil { + t.Fatal(err) + } + + cf, err = cm.GetFreezerState() + if err != nil { + t.Fatal(err) + } + if cf != configs.Thawed { + t.Fatalf("expected container to be thawed, got %v", cf) + } + + // Check the "container" works. + marker := "one two\n" + _, err = stdinW.WriteString(marker) + if err != nil { + t.Fatal(err) + } + reply, err := rdr.ReadString('\n') + if err != nil { + t.Fatalf("reading from container: %v", err) + } + if reply != marker { + t.Fatalf("expected %q, got %q", marker, reply) + } +} diff --git a/libcontainer/cgroups/systemd/systemd_test.go b/libcontainer/cgroups/systemd/systemd_test.go index 7417bf287..d280a3090 100644 --- a/libcontainer/cgroups/systemd/systemd_test.go +++ b/libcontainer/cgroups/systemd/systemd_test.go @@ -1,16 +1,11 @@ package systemd import ( - "bufio" - "bytes" "os" - "os/exec" - "strings" "testing" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" - "github.com/opencontainers/runc/libcontainer/devices" ) func newManager(t *testing.T, config *configs.Cgroup) (m cgroups.Manager) { @@ -74,228 +69,6 @@ func TestValidUnitTypes(t *testing.T) { } } -// TestPodSkipDevicesUpdate checks that updating a pod having SkipDevices: true -// does not result in spurious "permission denied" errors in a container -// running under the pod. The test is somewhat similar in nature to the -// @test "update devices [minimal transition rules]" in tests/integration, -// but uses a pod. -func TestPodSkipDevicesUpdate(t *testing.T) { - if !IsRunningSystemd() { - t.Skip("Test requires systemd.") - } - if os.Geteuid() != 0 { - t.Skip("Test requires root.") - } - - podName := "system-runc_test_pod" + t.Name() + ".slice" - podConfig := &configs.Cgroup{ - Systemd: true, - Parent: "system.slice", - Name: podName, - Resources: &configs.Resources{ - PidsLimit: 42, - Memory: 32 * 1024 * 1024, - SkipDevices: true, - }, - } - // Create "pod" cgroup (a systemd slice to hold containers). - pm := newManager(t, podConfig) - if err := pm.Apply(-1); err != nil { - t.Fatal(err) - } - if err := pm.Set(podConfig.Resources); err != nil { - t.Fatal(err) - } - - containerConfig := &configs.Cgroup{ - Parent: podName, - ScopePrefix: "test", - Name: "PodSkipDevicesUpdate", - Resources: &configs.Resources{ - Devices: []*devices.Rule{ - // Allow access to /dev/null. - { - Type: devices.CharDevice, - Major: 1, - Minor: 3, - Permissions: "rwm", - Allow: true, - }, - }, - }, - } - - // Create a "container" within the "pod" cgroup. - // This is not a real container, just a process in the cgroup. - cmd := exec.Command("bash", "-c", "while true; do echo > /dev/null; done") - cmd.Env = append(os.Environ(), "LANG=C") - var stderr bytes.Buffer - cmd.Stderr = &stderr - if err := cmd.Start(); err != nil { - t.Fatal(err) - } - // Make sure to not leave a zombie. - defer func() { - // These may fail, we don't care. - _ = cmd.Process.Kill() - _ = cmd.Wait() - }() - - // Put the process into a cgroup. - cm := newManager(t, containerConfig) - if err := cm.Apply(cmd.Process.Pid); err != nil { - t.Fatal(err) - } - // Check that we put the "container" into the "pod" cgroup. - if !strings.HasPrefix(cm.Path("devices"), pm.Path("devices")) { - t.Fatalf("expected container cgroup path %q to be under pod cgroup path %q", - cm.Path("devices"), pm.Path("devices")) - } - if err := cm.Set(containerConfig.Resources); err != nil { - t.Fatal(err) - } - - // Now update the pod a few times. - for i := 0; i < 42; i++ { - podConfig.Resources.PidsLimit++ - podConfig.Resources.Memory += 1024 * 1024 - if err := pm.Set(podConfig.Resources); err != nil { - t.Fatal(err) - } - } - // Kill the "container". - if err := cmd.Process.Kill(); err != nil { - t.Fatal(err) - } - - _ = cmd.Wait() - - // "Container" stderr should be empty. - if stderr.Len() != 0 { - t.Fatalf("container stderr not empty: %s", stderr.String()) - } -} - -func testSkipDevices(t *testing.T, skipDevices bool, expected []string) { - if !IsRunningSystemd() { - t.Skip("Test requires systemd.") - } - if os.Geteuid() != 0 { - t.Skip("Test requires root.") - } - - podConfig := &configs.Cgroup{ - Parent: "system.slice", - Name: "system-runc_test_pods.slice", - Resources: &configs.Resources{ - SkipDevices: skipDevices, - }, - } - // Create "pods" cgroup (a systemd slice to hold containers). - pm := newManager(t, podConfig) - if err := pm.Apply(-1); err != nil { - t.Fatal(err) - } - if err := pm.Set(podConfig.Resources); err != nil { - t.Fatal(err) - } - - config := &configs.Cgroup{ - Parent: "system-runc_test_pods.slice", - ScopePrefix: "test", - Name: "SkipDevices", - Resources: &configs.Resources{ - Devices: []*devices.Rule{ - // Allow access to /dev/full only. - { - Type: devices.CharDevice, - Major: 1, - Minor: 7, - Permissions: "rwm", - Allow: true, - }, - }, - }, - } - - // Create a "container" within the "pods" cgroup. - // This is not a real container, just a process in the cgroup. - cmd := exec.Command("bash", "-c", "read; echo > /dev/full; cat /dev/null; true") - cmd.Env = append(os.Environ(), "LANG=C") - stdinR, stdinW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - cmd.Stdin = stdinR - var stderr bytes.Buffer - cmd.Stderr = &stderr - err = cmd.Start() - stdinR.Close() - defer stdinW.Close() - if err != nil { - t.Fatal(err) - } - // Make sure to not leave a zombie. - defer func() { - // These may fail, we don't care. - _, _ = stdinW.WriteString("hey\n") - _ = cmd.Wait() - }() - - // Put the process into a cgroup. - m := newManager(t, config) - if err := m.Apply(cmd.Process.Pid); err != nil { - t.Fatal(err) - } - // Check that we put the "container" into the "pod" cgroup. - if !strings.HasPrefix(m.Path("devices"), pm.Path("devices")) { - t.Fatalf("expected container cgroup path %q to be under pod cgroup path %q", - m.Path("devices"), pm.Path("devices")) - } - if err := m.Set(config.Resources); err != nil { - // failed to write "c 1:7 rwm": write /sys/fs/cgroup/devices/system.slice/system-runc_test_pods.slice/test-SkipDevices.scope/devices.allow: operation not permitted - if skipDevices == false && strings.HasSuffix(err.Error(), "/devices.allow: operation not permitted") { - // Cgroup v1 devices controller gives EPERM on trying - // to enable devices that are not enabled - // (skipDevices=false) in a parent cgroup. - // If this happens, test is passing. - return - } - t.Fatal(err) - } - - // Check that we can access /dev/full but not /dev/zero. - if _, err := stdinW.WriteString("wow\n"); err != nil { - t.Fatal(err) - } - if err := cmd.Wait(); err != nil { - t.Fatal(err) - } - for _, exp := range expected { - if !strings.Contains(stderr.String(), exp) { - t.Errorf("expected %q, got: %s", exp, stderr.String()) - } - } -} - -func TestSkipDevicesTrue(t *testing.T) { - testSkipDevices(t, true, []string{ - "echo: write error: No space left on device", - "cat: /dev/null: Operation not permitted", - }) -} - -func TestSkipDevicesFalse(t *testing.T) { - // If SkipDevices is not set for the parent slice, access to both - // devices should fail. This is done to assess the test correctness. - // For cgroup v1, we check for m.Set returning EPERM. - // For cgroup v2, we check for the errors below. - testSkipDevices(t, false, []string{ - "/dev/full: Operation not permitted", - "cat: /dev/null: Operation not permitted", - }) -} - func TestUnitExistsIgnored(t *testing.T) { if !IsRunningSystemd() { t.Skip("Test requires systemd.") @@ -319,138 +92,3 @@ func TestUnitExistsIgnored(t *testing.T) { } } } - -func TestFreezePodCgroup(t *testing.T) { - if !IsRunningSystemd() { - t.Skip("Test requires systemd.") - } - if os.Geteuid() != 0 { - t.Skip("Test requires root.") - } - - podConfig := &configs.Cgroup{ - Parent: "system.slice", - Name: "system-runc_test_pod.slice", - Resources: &configs.Resources{ - SkipDevices: true, - Freezer: configs.Frozen, - }, - } - // Create a "pod" cgroup (a systemd slice to hold containers), - // which is frozen initially. - pm := newManager(t, podConfig) - if err := pm.Apply(-1); err != nil { - t.Fatal(err) - } - - if err := pm.Set(podConfig.Resources); err != nil { - t.Fatal(err) - } - - // Check the pod is frozen. - pf, err := pm.GetFreezerState() - if err != nil { - t.Fatal(err) - } - if pf != configs.Frozen { - t.Fatalf("expected pod to be frozen, got %v", pf) - } - - // Create a "container" within the "pod" cgroup. - // This is not a real container, just a process in the cgroup. - containerConfig := &configs.Cgroup{ - Parent: "system-runc_test_pod.slice", - ScopePrefix: "test", - Name: "inner-container", - Resources: &configs.Resources{}, - } - - cmd := exec.Command("bash", "-c", "while read; do echo $REPLY; done") - cmd.Env = append(os.Environ(), "LANG=C") - - // Setup stdin. - stdinR, stdinW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - cmd.Stdin = stdinR - - // Setup stdout. - stdoutR, stdoutW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - cmd.Stdout = stdoutW - rdr := bufio.NewReader(stdoutR) - - // Setup stderr. - var stderr bytes.Buffer - cmd.Stderr = &stderr - - err = cmd.Start() - stdinR.Close() - stdoutW.Close() - defer func() { - _ = stdinW.Close() - _ = stdoutR.Close() - }() - if err != nil { - t.Fatal(err) - } - // Make sure to not leave a zombie. - defer func() { - // These may fail, we don't care. - _ = cmd.Process.Kill() - _ = cmd.Wait() - }() - - // Put the process into a cgroup. - cm := newManager(t, containerConfig) - - if err := cm.Apply(cmd.Process.Pid); err != nil { - t.Fatal(err) - } - if err := cm.Set(containerConfig.Resources); err != nil { - t.Fatal(err) - } - // Check that we put the "container" into the "pod" cgroup. - if !strings.HasPrefix(cm.Path("freezer"), pm.Path("freezer")) { - t.Fatalf("expected container cgroup path %q to be under pod cgroup path %q", - cm.Path("freezer"), pm.Path("freezer")) - } - // Check the container is not reported as frozen despite the frozen parent. - cf, err := cm.GetFreezerState() - if err != nil { - t.Fatal(err) - } - if cf != configs.Thawed { - t.Fatalf("expected container to be thawed, got %v", cf) - } - - // Unfreeze the pod. - if err := pm.Freeze(configs.Thawed); err != nil { - t.Fatal(err) - } - - cf, err = cm.GetFreezerState() - if err != nil { - t.Fatal(err) - } - if cf != configs.Thawed { - t.Fatalf("expected container to be thawed, got %v", cf) - } - - // Check the "container" works. - marker := "one two\n" - _, err = stdinW.WriteString(marker) - if err != nil { - t.Fatal(err) - } - reply, err := rdr.ReadString('\n') - if err != nil { - t.Fatalf("reading from container: %v", err) - } - if reply != marker { - t.Fatalf("expected %q, got %q", marker, reply) - } -} diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index a74a05a5c..1e4c58212 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -4,12 +4,10 @@ import ( "errors" "os" "path/filepath" - "reflect" "strings" "sync" systemdDbus "github.com/coreos/go-systemd/v22/dbus" - "github.com/godbus/dbus/v5" "github.com/sirupsen/logrus" "github.com/opencontainers/runc/libcontainer/cgroups" @@ -336,71 +334,6 @@ func (m *legacyManager) GetStats() (*cgroups.Stats, error) { return stats, nil } -// freezeBeforeSet answers whether there is a need to freeze the cgroup before -// applying its systemd unit properties, and thaw after, while avoiding -// unnecessary freezer state changes. -// -// The reason why we have to freeze is that systemd's application of device -// rules is done disruptively, resulting in spurious errors to common devices -// (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) { - // Special case for SkipDevices, as used by Kubernetes to create pod - // cgroups with allow-all device policy). - if r.SkipDevices { - if r.SkipFreezeOnSet { - // Both needsFreeze and needsThaw are false. - return - } - - // No need to freeze if SkipDevices is set, and either - // (1) systemd unit does not (yet) exist, or - // (2) it has DevicePolicy=auto and empty DeviceAllow list. - // - // Interestingly, (1) and (2) are the same here because - // a non-existent unit returns default properties, - // and settings in (2) are the defaults. - // - // Do not return errors from getUnitTypeProperty, as they alone - // should not prevent Set from working. - - unitType := getUnitType(unitName) - - devPolicy, e := getUnitTypeProperty(m.dbus, unitName, unitType, "DevicePolicy") - if e == nil && devPolicy.Value == dbus.MakeVariant("auto") { - devAllow, e := getUnitTypeProperty(m.dbus, unitName, unitType, "DeviceAllow") - if e == nil { - if rv := reflect.ValueOf(devAllow.Value.Value()); rv.Kind() == reflect.Slice && rv.Len() == 0 { - needsFreeze = false - needsThaw = false - return - } - } - } - } - - needsFreeze = true - needsThaw = true - - // Check the current freezer state. - freezerState, err := m.GetFreezerState() - if err != nil { - return - } - if freezerState == configs.Frozen { - // Already frozen, and should stay frozen. - needsFreeze = false - needsThaw = false - } - - if r.Freezer == configs.Frozen { - // Will be frozen anyway -- no need to thaw. - needsThaw = false - } - return -} - func (m *legacyManager) Set(r *configs.Resources) error { if r == nil { return nil diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index 10ca0eeea..17b05a552 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -12,6 +12,8 @@ import ( securejoin "github.com/cyphar/filepath-securejoin" "golang.org/x/sys/unix" + //nolint:revive // Enable cgroup manager to manage devices + _ "github.com/opencontainers/runc/libcontainer/cgroups/devices" "github.com/opencontainers/runc/libcontainer/cgroups/manager" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/configs/validate" diff --git a/libcontainer/integration/init_test.go b/libcontainer/integration/init_test.go index b6a3714d4..76638ffc7 100644 --- a/libcontainer/integration/init_test.go +++ b/libcontainer/integration/init_test.go @@ -6,6 +6,8 @@ import ( "testing" "github.com/opencontainers/runc/libcontainer" + //nolint:revive // Enable cgroup manager to manage devices + _ "github.com/opencontainers/runc/libcontainer/cgroups/devices" _ "github.com/opencontainers/runc/libcontainer/nsenter" "github.com/sirupsen/logrus"