From 09b80811f63bf562dd60653ed80b98ea4e627c67 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 25 Aug 2021 14:11:32 +1000 Subject: [PATCH] Revert "libct/devices: change devices.Type to be a string" This reverts commit 814f3ae1d93d35280eefd359be73f473519e7036. This changed the on-disk state which breaks runc when it has to operate on containers started with an older runc version. Working around this is far more complicated than just reverting it. Signed-off-by: Aleksa Sarai --- .../cgroups/devices/devices_emulator.go | 15 +++++----- .../cgroups/ebpf/devicefilter/devicefilter.go | 2 +- .../ebpf/devicefilter/devicefilter_test.go | 12 ++++---- libcontainer/devices/device.go | 12 ++++---- libcontainer/rootfs_linux.go | 2 +- libcontainer/specconv/spec_linux.go | 30 ++++++++++++++----- 6 files changed, 44 insertions(+), 29 deletions(-) diff --git a/libcontainer/cgroups/devices/devices_emulator.go b/libcontainer/cgroups/devices/devices_emulator.go index b5ff2dfe9..d50fc91b4 100644 --- a/libcontainer/cgroups/devices/devices_emulator.go +++ b/libcontainer/cgroups/devices/devices_emulator.go @@ -89,7 +89,7 @@ func parseLine(line string) (*deviceRule, error) { var ( rule deviceRule - node = devices.Type(fields[0]) + node = fields[0] major = fields[1] minor = fields[2] perms = fields[3] @@ -97,14 +97,16 @@ func parseLine(line string) (*deviceRule, error) { // Parse the node type. switch node { - case devices.WildcardDevice: + case "a": // Super-special case -- "a" always means every device with every // access mode. In fact, for devices.list this actually indicates that // the cgroup is in black-list mode. // TODO: Double-check that the entire file is "a *:* rwm". return nil, nil - case devices.BlockDevice, devices.CharDevice: - rule.meta.node = node + case "b": + rule.meta.node = devices.BlockDevice + case "c": + rule.meta.node = devices.CharDevice default: return nil, fmt.Errorf("unknown device type %q", node) } @@ -136,7 +138,6 @@ func parseLine(line string) (*deviceRule, error) { if !rule.perms.IsValid() || rule.perms.IsEmpty() { return nil, fmt.Errorf("parse access mode: contained unknown modes or is empty: %q", perms) } - return &rule, nil } @@ -318,10 +319,10 @@ func (source *Emulator) Transition(target *Emulator) ([]*devices.Rule, error) { // black-list we also have to include a disruptive rule. if source.IsBlacklist() || source.defaultAllow != target.defaultAllow { transitionRules = append(transitionRules, &devices.Rule{ - Type: devices.WildcardDevice, + Type: 'a', Major: -1, Minor: -1, - Permissions: "rwm", + Permissions: devices.Permissions("rwm"), Allow: target.defaultAllow, }) // The old rules are only relevant if we aren't starting out with a diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go index 94f3e9305..9874a175f 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go +++ b/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go @@ -119,7 +119,7 @@ func (p *program) appendRule(rule *devices.Rule) error { bpfType = int32(unix.BPF_DEVCG_DEV_BLOCK) default: // We do not permit 'a', nor any other types we don't know about. - return fmt.Errorf("invalid type %q", rule.Type) + return fmt.Errorf("invalid type %q", string(rule.Type)) } if rule.Major > math.MaxUint32 { return fmt.Errorf("invalid major %d", rule.Major) diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go index a78f15310..a8fc562d0 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go +++ b/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go @@ -146,7 +146,7 @@ block-11: func TestDeviceFilter_Privileged(t *testing.T) { devices := []*devices.Rule{ { - Type: devices.WildcardDevice, + Type: 'a', Major: -1, Minor: -1, Permissions: "rwm", @@ -173,14 +173,14 @@ block-0: func TestDeviceFilter_PrivilegedExceptSingleDevice(t *testing.T) { devices := []*devices.Rule{ { - Type: devices.WildcardDevice, + Type: 'a', Major: -1, Minor: -1, Permissions: "rwm", Allow: true, }, { - Type: devices.BlockDevice, + Type: 'b', Major: 8, Minor: 0, Permissions: "rwm", @@ -213,21 +213,21 @@ block-1: func TestDeviceFilter_Weird(t *testing.T) { devices := []*devices.Rule{ { - Type: devices.BlockDevice, + Type: 'b', Major: 8, Minor: 1, Permissions: "rwm", Allow: false, }, { - Type: devices.WildcardDevice, + Type: 'a', Major: -1, Minor: -1, Permissions: "rwm", Allow: true, }, { - Type: devices.BlockDevice, + Type: 'b', Major: 8, Minor: 2, Permissions: "rwm", diff --git a/libcontainer/devices/device.go b/libcontainer/devices/device.go index 5a48245d1..c2c2b3bb7 100644 --- a/libcontainer/devices/device.go +++ b/libcontainer/devices/device.go @@ -100,13 +100,13 @@ func (p Permissions) IsValid() bool { return p == fromSet(p.toSet()) } -type Type string +type Type rune const ( - WildcardDevice Type = "a" - BlockDevice Type = "b" - CharDevice Type = "c" // or 'u' - FifoDevice Type = "p" + WildcardDevice Type = 'a' + BlockDevice Type = 'b' + CharDevice Type = 'c' // or 'u' + FifoDevice Type = 'p' ) func (t Type) IsValid() bool { @@ -166,7 +166,7 @@ func (d *Rule) CgroupString() string { if d.Minor == Wildcard { minor = "*" } - return fmt.Sprintf("%s %s:%s %s", d.Type, major, minor, d.Permissions) + return fmt.Sprintf("%c %s:%s %s", d.Type, major, minor, d.Permissions) } func (d *Rule) Mkdev() (uint64, error) { diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 497e04f29..3c3fc0d11 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -710,7 +710,7 @@ func mknodDevice(dest string, node *devices.Device) error { case devices.FifoDevice: fileMode |= unix.S_IFIFO default: - return fmt.Errorf("%s is not a valid device type for device %s", node.Type, node.Path) + return fmt.Errorf("%c is not a valid device type for device %s", node.Type, node.Path) } dev, err := node.Mkdev() if err != nil { diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index e3b7ed9e1..891723163 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -479,15 +479,12 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi if r != nil { for i, d := range spec.Linux.Resources.Devices { var ( - dt = devices.WildcardDevice + t = "a" major = int64(-1) minor = int64(-1) ) if d.Type != "" { - dt = devices.Type(d.Type) - if !dt.CanCgroup() { - return nil, fmt.Errorf("invalid cgroup device type %q", d.Type) - } + t = d.Type } if d.Major != nil { major = *d.Major @@ -498,6 +495,10 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi if d.Access == "" { return nil, fmt.Errorf("device access at %d field cannot be empty", i) } + dt, err := stringToCgroupDeviceRune(t) + if err != nil { + return nil, err + } c.Resources.Devices = append(c.Resources.Devices, &devices.Rule{ Type: dt, Major: major, @@ -634,7 +635,20 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi return c, nil } -func stringToDeviceType(s string) (devices.Type, error) { +func stringToCgroupDeviceRune(s string) (devices.Type, error) { + switch s { + case "a": + return devices.WildcardDevice, nil + case "b": + return devices.BlockDevice, nil + case "c": + return devices.CharDevice, nil + default: + return 0, fmt.Errorf("invalid cgroup device type %q", s) + } +} + +func stringToDeviceRune(s string) (devices.Type, error) { switch s { case "p": return devices.FifoDevice, nil @@ -643,7 +657,7 @@ func stringToDeviceType(s string) (devices.Type, error) { case "b": return devices.BlockDevice, nil default: - return "", fmt.Errorf("invalid device type %q", s) + return 0, fmt.Errorf("invalid device type %q", s) } } @@ -679,7 +693,7 @@ next: if d.GID != nil { gid = *d.GID } - dt, err := stringToDeviceType(d.Type) + dt, err := stringToDeviceRune(d.Type) if err != nil { return nil, err }