From 6c5441e5cb85907ce0c4dd57cac42daaf2f90485 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Tue, 10 Aug 2021 17:02:42 -0700
Subject: [PATCH] libct/cg/fs: move paths init to NewManager

1. Separate path initialization logic from Apply to initPaths,
   and call initPaths from NewManager, so:
   - we can error out early (in NewManager rather than Apply);
   - always have m.paths available (e.g. in Destroy or Exists).
   - do not unnecessarily call subsysPath from Apply in case
     the paths were already provided.

2. Add a check for non-nil cgroups.Resources to NewManager,
   since initPaths, as well as some controller's Apply methods,
   need it.

3. Move cgroups.Resources.Unified check from Apply to NewManager,
   so we can error out early (same check exists in Set).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 libcontainer/cgroups/fs/fs.go      | 45 +++++++++++++++---------------
 libcontainer/cgroups/fs/paths.go   | 30 ++++++++++++++++++++
 libcontainer/factory_linux_test.go |  8 ++++--
 3 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go
index e40abb45b..8976c4f9f 100644
--- a/libcontainer/cgroups/fs/fs.go
+++ b/libcontainer/cgroups/fs/fs.go
@@ -55,6 +55,23 @@ type manager struct {
 }
 
 func NewManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Manager, error) {
+	// Some v1 controllers (cpu, cpuset, and devices) expect
+	// cgroups.Resources to not be nil in Apply.
+	if cg.Resources == nil {
+		return nil, errors.New("cgroup v1 manager needs configs.Resources to be set during manager creation")
+	}
+	if cg.Resources.Unified != nil {
+		return nil, cgroups.ErrV1NoUnified
+	}
+
+	if paths == nil {
+		var err error
+		paths, err = initPaths(cg)
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	return &manager{
 		cgroups: cg,
 		paths:   paths,
@@ -90,37 +107,21 @@ func (m *manager) Apply(pid int) (err error) {
 	defer m.mu.Unlock()
 
 	c := m.cgroups
-	if c.Resources.Unified != nil {
-		return cgroups.ErrV1NoUnified
-	}
 
-	root, err := rootPath()
-	if err != nil {
-		return err
-	}
-
-	inner, err := innerPath(c)
-
-	m.paths = make(map[string]string)
 	for _, sys := range subsystems {
-		p, err := subsysPath(root, inner, sys.Name())
-		if err != nil {
-			// The non-presence of the devices subsystem is
-			// considered fatal for security reasons.
-			if cgroups.IsNotFound(err) && (c.SkipDevices || sys.Name() != "devices") {
-				continue
-			}
-			return err
+		name := sys.Name()
+		p, ok := m.paths[name]
+		if !ok {
+			continue
 		}
-		m.paths[sys.Name()] = p
 
 		if err := sys.Apply(p, c.Resources, pid); err != nil {
 			// In the case of rootless (including euid=0 in userns), where an
 			// explicit cgroup path hasn't been set, we don't bail on error in
 			// case of permission problems. Cases where limits have been set
 			// (and we couldn't create our own cgroup) are handled by Set.
-			if isIgnorableError(c.Rootless, err) && m.cgroups.Path == "" {
-				delete(m.paths, sys.Name())
+			if isIgnorableError(c.Rootless, err) && c.Path == "" {
+				delete(m.paths, name)
 				continue
 			}
 			return err
diff --git a/libcontainer/cgroups/fs/paths.go b/libcontainer/cgroups/fs/paths.go
index fb4ff8b80..1092331b2 100644
--- a/libcontainer/cgroups/fs/paths.go
+++ b/libcontainer/cgroups/fs/paths.go
@@ -21,6 +21,36 @@ var (
 
 const defaultCgroupRoot = "/sys/fs/cgroup"
 
+func initPaths(cg *configs.Cgroup) (map[string]string, error) {
+	root, err := rootPath()
+	if err != nil {
+		return nil, err
+	}
+
+	inner, err := innerPath(cg)
+	if err != nil {
+		return nil, err
+	}
+
+	paths := make(map[string]string)
+	for _, sys := range subsystems {
+		name := sys.Name()
+		path, err := subsysPath(root, inner, name)
+		if err != nil {
+			// The non-presence of the devices subsystem
+			// is considered fatal for security reasons.
+			if cgroups.IsNotFound(err) && (cg.SkipDevices || name != "devices") {
+				continue
+			}
+
+			return nil, err
+		}
+		paths[name] = path
+	}
+
+	return paths, nil
+}
+
 func tryDefaultCgroupRoot() string {
 	var st, pst unix.Stat_t
 
diff --git a/libcontainer/factory_linux_test.go b/libcontainer/factory_linux_test.go
index 377bcea90..d29c32e9d 100644
--- a/libcontainer/factory_linux_test.go
+++ b/libcontainer/factory_linux_test.go
@@ -138,9 +138,11 @@ func TestFactoryLoadContainer(t *testing.T) {
 			},
 		}
 		expectedConfig = &configs.Config{
-			Rootfs:  "/mycontainer/root",
-			Hooks:   expectedHooks,
-			Cgroups: &configs.Cgroup{},
+			Rootfs: "/mycontainer/root",
+			Hooks:  expectedHooks,
+			Cgroups: &configs.Cgroup{
+				Resources: &configs.Resources{},
+			},
 		}
 		expectedState = &State{
 			BaseState: BaseState{