libct/cg/sd: error on untranslatable dev rules in v2

It seems that the code added by commit b810da1 had cgroup v1
in mind, where runc overwrites the rules set by systemd. It is different
in v2, because both ebpf programs (systemd's and runc's) has to return
"allow" for the device to get access.

So, when using cgroup v2 and systemd cgroup driver, access to devices
rules for that can't be translated to systemd properties is not
possible, and it makes sense to error out (rather than warn) in such
case, as the container won't work as intended.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Author: kolyshkin

libcontainer/cgroups/devices/systemd.go

@@ -17,7 +17,7 @@ import (
 
 // systemdProperties takes the configured device rules and generates a
 // corresponding set of systemd properties to configure the devices correctly.
-func systemdProperties(r *configs.Resources, sdVer int) ([]systemdDbus.Property, error) {
+func systemdProperties(r *configs.Resources, sdVer int, cgroupVer int) ([]systemdDbus.Property, error) {
 	if r.SkipDevices {
 		return nil, nil
 	}
@@ -95,12 +95,33 @@ func systemdProperties(r *configs.Resources, sdVer int) ([]systemdDbus.Property,
 		// 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.
+		//
+		// The rules generated here are applied by systemd, and then runc applies
+		// its own rules on top of those. If some rules can not be converted to
+		// systemd properties, there will be two different sets of rules, and
+		// the end result depends on cgroup version:
+		//
+		//  * In cgroup v1 ("device.{allow,deny}" files), the rules set by systemd
+		//    gets overwritten by runc, meaning the correct rules are in effect.
+		//    Therefore, a warning is sufficient here (except for the case when
+		//    systemd is restarted and our rules gets overwritten, but there
+		//    is nothing we can do here).
+		//
+		//  * In cgroup v2 (ebpf), both systemd and runc ebpf programs are loaded,
+		//    and the device access is granted only if both programs return "allow".
+		//    Therefore, the rules that can not be translated to systemd properties
+		//    won't work, so we must return an error.
 
 		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)
+				w := fmt.Errorf("systemd does not support wildcard-major device rule: %+v", *rule)
+				if cgroupVer == 2 {
+					return nil, w
+				}
+				logrus.Warnf("temporarily ignoring rule: %v", w)
 				continue
+
 			}
 
 			// "_ *:* _" rules just wildcard everything.
@@ -117,7 +138,11 @@ func systemdProperties(r *configs.Resources, sdVer int) ([]systemdDbus.Property,
 			}
 			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)
+				w := fmt.Errorf("systemd older than v240 does not support wildcard-minor rules for devices not listed in /proc/devices: +%v", *rule)
+				if cgroupVer == 2 {
+					return nil, err
+				}
+				logrus.Warnf("temporarily ignoring rule: %v", w)
 				continue
 			}
 			entry.Path = group
@@ -131,12 +156,13 @@ func systemdProperties(r *configs.Resources, sdVer int) ([]systemdDbus.Property,
 			}
 			if sdVer < 240 {
 				// Old systemd versions use stat(2) on path to find out device major:minor
-				// numbers and type. If the path doesn't exist, it will not add the rule,
-				// emitting a warning instead.
-				// Since all of this logic is best-effort anyway (we manually set these
-				// rules separately to systemd) we can safely skip entries that don't
-				// have a corresponding path.
+				// numbers and type. If the path doesn't exist, it will not add the rule.
 				if _, err := os.Stat(entry.Path); err != nil {
+					w := fmt.Errorf("systemd older than v240 does not support device rules for non-existing device: %v", err)
+					if cgroupVer == 2 {
+						return nil, err
+					}
+					logrus.Warnf("temporarily ignoring rule: %v", w)
 					continue
 				}
 			}

libcontainer/cgroups/systemd/common.go

@@ -33,7 +33,7 @@ var (
 	isRunningSystemdOnce sync.Once
 	isRunningSystemd     bool
 
-	GenerateDeviceProps func(r *configs.Resources, sdVer int) ([]systemdDbus.Property, error)
+	GenerateDeviceProps func(r *configs.Resources, sdVer, cgroupVer int) ([]systemdDbus.Property, error)
 )
 
 // NOTE: This function comes from package github.com/coreos/go-systemd/util
@@ -342,13 +342,13 @@ func addCpuset(cm *dbusConnManager, props *[]systemdDbus.Property, cpus, mems st
 
 // generateDeviceProperties takes the configured device rules and generates a
 // corresponding set of systemd properties to configure the devices correctly.
-func generateDeviceProperties(r *configs.Resources, cm *dbusConnManager) ([]systemdDbus.Property, error) {
+func generateDeviceProperties(r *configs.Resources, cm *dbusConnManager, cgroupVersion int) ([]systemdDbus.Property, error) {
 	if GenerateDeviceProps == nil {
 		if len(r.Devices) > 0 {
 			return nil, cgroups.ErrDevicesUnsupported
 		}
 		return nil, nil
 	}
 
-	return GenerateDeviceProps(r, systemdVersion(cm))
+	return GenerateDeviceProps(r, systemdVersion(cm), cgroupVersion)
 }

libcontainer/cgroups/systemd/v1.go

@@ -75,7 +75,7 @@ var legacySubsystems = []subsystem{
 func genV1ResourcesProperties(r *configs.Resources, cm *dbusConnManager) ([]systemdDbus.Property, error) {
 	var properties []systemdDbus.Property
 
-	deviceProperties, err := generateDeviceProperties(r, cm)
+	deviceProperties, err := generateDeviceProperties(r, cm, 1)
 	if err != nil {
 		return nil, err
 	}

libcontainer/cgroups/systemd/v2.go

@@ -214,7 +214,7 @@ func genV2ResourcesProperties(dirPath string, r *configs.Resources, cm *dbusConn
 	//       aren't the end of the world, but it is a bit concerning. However
 	//       it's unclear if systemd removes all eBPF programs attached when
 	//       doing SetUnitProperties...
-	deviceProperties, err := generateDeviceProperties(r, cm)
+	deviceProperties, err := generateDeviceProperties(r, cm, 2)
 	if err != nil {
 		return nil, err
 	}