From afe83489d4243800cb68b5c766831a4ceaff4578 Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <asarai@suse.de>
Date: Sat, 9 May 2020 02:17:08 +1000
Subject: [PATCH] cgroupv1: devices: use minimal transition rules with
 devices.Emulator

Now that all of the infrastructure for devices.Emulator is in place, we
can finally implement minimal transition rules for devices cgroups. This
allows for minimal disruption to running containers if a rule update is
requested. Only in very rare circumstances (black-list cgroups and mode
switching) will a clear-all rule be written. As a result, containers
should no longer see spurious errors.

A similar issue affects the cgroupv2 devices setup, but that is a topic
for another time (as the solution is drastically different).

Signed-off-by: Aleksa Sarai <asarai@suse.de>
---
 libcontainer/cgroups/fs/devices.go      | 69 +++++++++++++++++++++----
 libcontainer/cgroups/fs/devices_test.go |  3 +-
 2 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/libcontainer/cgroups/fs/devices.go b/libcontainer/cgroups/fs/devices.go
index 164bf326f..4fc3951de 100644
--- a/libcontainer/cgroups/fs/devices.go
+++ b/libcontainer/cgroups/fs/devices.go
@@ -3,13 +3,19 @@
 package fs
 
 import (
+	"bytes"
+	"fmt"
+	"reflect"
+
 	"github.com/opencontainers/runc/libcontainer/cgroups"
+	"github.com/opencontainers/runc/libcontainer/cgroups/devices"
 	"github.com/opencontainers/runc/libcontainer/cgroups/fscommon"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runc/libcontainer/system"
 )
 
 type DevicesGroup struct {
+	testingSkipFinalCheck bool
 }
 
 func (s *DevicesGroup) Name() string {
@@ -26,29 +32,74 @@ func (s *DevicesGroup) Apply(d *cgroupData) error {
 	return nil
 }
 
+func loadEmulator(path string) (*devices.Emulator, error) {
+	list, err := fscommon.ReadFile(path, "devices.list")
+	if err != nil {
+		return nil, err
+	}
+	return devices.EmulatorFromList(bytes.NewBufferString(list))
+}
+
+func buildEmulator(rules []*configs.DeviceRule) (*devices.Emulator, error) {
+	// This defaults to a white-list -- which is what we want!
+	emu := &devices.Emulator{}
+	for _, rule := range rules {
+		if err := emu.Apply(*rule); err != nil {
+			return nil, err
+		}
+	}
+	return emu, nil
+}
+
 func (s *DevicesGroup) Set(path string, cgroup *configs.Cgroup) error {
 	if system.RunningInUserNS() {
 		return nil
 	}
 
-	// The devices list is a whitelist, so we must first deny all devices.
-	// XXX: This is incorrect for device list updates as it will result in
-	//      spurrious errors in the container, but we will solve that
-	//      separately.
-	if err := fscommon.WriteFile(path, "devices.deny", "a"); err != 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(cgroup.Resources.Devices)
+	if err != nil {
 		return err
 	}
 
-	devices := cgroup.Resources.Devices
-	for _, dev := range devices {
+	// 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 dev.Allow {
+		if rule.Allow {
 			file = "devices.allow"
 		}
-		if err := fscommon.WriteFile(path, file, dev.CgroupString()); err != nil {
+		if err := fscommon.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 !s.testingSkipFinalCheck {
+		currentAfter, err := loadEmulator(path)
+		if err != nil {
+			return err
+		}
+		if !target.IsBlacklist() && !reflect.DeepEqual(currentAfter, target) {
+			return fmt.Errorf("resulting devices cgroup doesn't precisely match target")
+		} else if target.IsBlacklist() != currentAfter.IsBlacklist() {
+			return fmt.Errorf("resulting devices cgroup doesn't match target mode")
+		}
+	}
 	return nil
 }
 
diff --git a/libcontainer/cgroups/fs/devices_test.go b/libcontainer/cgroups/fs/devices_test.go
index 77e29124c..da7ccb01e 100644
--- a/libcontainer/cgroups/fs/devices_test.go
+++ b/libcontainer/cgroups/fs/devices_test.go
@@ -16,6 +16,7 @@ func TestDevicesSetAllow(t *testing.T) {
 	helper.writeFileContents(map[string]string{
 		"devices.allow": "",
 		"devices.deny":  "",
+		"devices.list":  "a *:* rwm",
 	})
 
 	helper.CgroupData.config.Resources.Devices = []*configs.DeviceRule{
@@ -28,7 +29,7 @@ func TestDevicesSetAllow(t *testing.T) {
 		},
 	}
 
-	devices := &DevicesGroup{}
+	devices := &DevicesGroup{testingSkipFinalCheck: true}
 	if err := devices.Set(helper.CgroupPath, helper.CgroupData.config); err != nil {
 		t.Fatal(err)
 	}