From 814f3ae1d93d35280eefd359be73f473519e7036 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 13 Aug 2021 00:21:49 +0200 Subject: [PATCH] libct/devices: change devices.Type to be a string Possibly there was a specific reason to use a rune for this, but I noticed that there's various parts in the code that has to convert values from a string to this type. Using a string as type for this can simplify some of that code. Signed-off-by: Sebastiaan van Stijn --- .../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, 29 insertions(+), 44 deletions(-) diff --git a/libcontainer/cgroups/devices/devices_emulator.go b/libcontainer/cgroups/devices/devices_emulator.go index ebb8b6cd8..fb1ad14d2 100644 --- a/libcontainer/cgroups/devices/devices_emulator.go +++ b/libcontainer/cgroups/devices/devices_emulator.go @@ -88,7 +88,7 @@ func parseLine(line string) (*deviceRule, error) { } var ( rule deviceRule - node = matches[1] + node = devices.Type(matches[1]) major = matches[2] minor = matches[3] perms = matches[4] @@ -96,16 +96,14 @@ func parseLine(line string) (*deviceRule, error) { // Parse the node type. switch node { - case "a": + case devices.WildcardDevice: // 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 "b": - rule.meta.node = devices.BlockDevice - case "c": - rule.meta.node = devices.CharDevice + case devices.BlockDevice, devices.CharDevice: + rule.meta.node = node default: // Should never happen! return nil, fmt.Errorf("unknown device type %q", node) @@ -139,6 +137,7 @@ func parseLine(line string) (*deviceRule, error) { // Should never happen! return nil, fmt.Errorf("parse access mode: contained unknown modes or is empty: %q", perms) } + return &rule, nil } @@ -320,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: 'a', + Type: devices.WildcardDevice, Major: -1, Minor: -1, - Permissions: devices.Permissions("rwm"), + 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 9874a175f..94f3e9305 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", string(rule.Type)) + return fmt.Errorf("invalid type %q", 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 a8fc562d0..a78f15310 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: 'a', + Type: devices.WildcardDevice, Major: -1, Minor: -1, Permissions: "rwm", @@ -173,14 +173,14 @@ block-0: func TestDeviceFilter_PrivilegedExceptSingleDevice(t *testing.T) { devices := []*devices.Rule{ { - Type: 'a', + Type: devices.WildcardDevice, Major: -1, Minor: -1, Permissions: "rwm", Allow: true, }, { - Type: 'b', + Type: devices.BlockDevice, Major: 8, Minor: 0, Permissions: "rwm", @@ -213,21 +213,21 @@ block-1: func TestDeviceFilter_Weird(t *testing.T) { devices := []*devices.Rule{ { - Type: 'b', + Type: devices.BlockDevice, Major: 8, Minor: 1, Permissions: "rwm", Allow: false, }, { - Type: 'a', + Type: devices.WildcardDevice, Major: -1, Minor: -1, Permissions: "rwm", Allow: true, }, { - Type: 'b', + Type: devices.BlockDevice, Major: 8, Minor: 2, Permissions: "rwm", diff --git a/libcontainer/devices/device.go b/libcontainer/devices/device.go index c2c2b3bb7..5a48245d1 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 rune +type Type string 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("%c %s:%s %s", d.Type, major, minor, d.Permissions) + return fmt.Sprintf("%s %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 361ce9d3d..beae931de 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("%c is not a valid device type for device %s", node.Type, node.Path) + return fmt.Errorf("%s 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 891723163..e3b7ed9e1 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -479,12 +479,15 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi if r != nil { for i, d := range spec.Linux.Resources.Devices { var ( - t = "a" + dt = devices.WildcardDevice major = int64(-1) minor = int64(-1) ) if d.Type != "" { - t = d.Type + dt = devices.Type(d.Type) + if !dt.CanCgroup() { + return nil, fmt.Errorf("invalid cgroup device type %q", d.Type) + } } if d.Major != nil { major = *d.Major @@ -495,10 +498,6 @@ 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, @@ -635,20 +634,7 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi return c, nil } -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) { +func stringToDeviceType(s string) (devices.Type, error) { switch s { case "p": return devices.FifoDevice, nil @@ -657,7 +643,7 @@ func stringToDeviceRune(s string) (devices.Type, error) { case "b": return devices.BlockDevice, nil default: - return 0, fmt.Errorf("invalid device type %q", s) + return "", fmt.Errorf("invalid device type %q", s) } } @@ -693,7 +679,7 @@ next: if d.GID != nil { gid = *d.GID } - dt, err := stringToDeviceRune(d.Type) + dt, err := stringToDeviceType(d.Type) if err != nil { return nil, err }