cgroup: make paths available, simplify getting manager

contrib/cmd/sd-helper/helper.go

@@ -57,9 +57,9 @@ func main() {
 	}
 }
 
-func newManager(config *configs.Cgroup) cgroups.Manager {
+func newManager(config *configs.Cgroup) (cgroups.Manager, error) {
 	if cgroups.IsCgroup2UnifiedMode() {
-		return systemd.NewUnifiedManager(config, "", false)
+		return systemd.NewUnifiedManager(config, "")
 	}
 	return systemd.NewLegacyManager(config, nil)
 }
@@ -70,7 +70,10 @@ func unitCommand(cmd, name, parent string) error {
 		Parent:    parent,
 		Resources: &configs.Resources{},
 	}
-	pm := newManager(podConfig)
+	pm, err := newManager(podConfig)
+	if err != nil {
+		return err
+	}
 
 	switch cmd {
 	case "start":

libcontainer/cgroups/fs/fs.go

@@ -49,18 +49,33 @@ type subsystem interface {
 }
 
 type manager struct {
-	mu       sync.Mutex
-	cgroups  *configs.Cgroup
-	rootless bool // ignore permission-related errors
-	paths    map[string]string
+	mu      sync.Mutex
+	cgroups *configs.Cgroup
+	paths   map[string]string
 }
 
-func NewManager(cg *configs.Cgroup, paths map[string]string, rootless bool) cgroups.Manager {
-	return &manager{
-		cgroups:  cg,
-		paths:    paths,
-		rootless: rootless,
+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,
+	}, nil
 }
 
 // isIgnorableError returns whether err is a permission error (in the loose
@@ -85,44 +100,30 @@ func isIgnorableError(rootless bool, err error) bool {
 }
 
 func (m *manager) Apply(pid int) (err error) {
-	if m.cgroups == nil {
-		return nil
-	}
 	m.mu.Lock()
 	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(m.rootless, err) && m.cgroups.Path == "" {
-				delete(m.paths, sys.Name())
+			// case of permission problems here, but do delete the path from
+			// the m.paths map, since it is either non-existent and could not
+			// be created, or the pid could not be added to it.
+			//
+			// Cases where limits for the subsystem have been set are handled
+			// later by Set, which fails with a friendly error (see
+			// if path == "" in Set).
+			if isIgnorableError(c.Rootless, err) && c.Path == "" {
+				delete(m.paths, name)
 				continue
 			}
 			return err
@@ -174,10 +175,11 @@ func (m *manager) Set(r *configs.Resources) error {
 	for _, sys := range subsystems {
 		path := m.paths[sys.Name()]
 		if err := sys.Set(path, r); err != nil {
-			if m.rootless && sys.Name() == "devices" {
+			// When rootless is true, errors from the device subsystem
+			// are ignored, as it is really not expected to work.
+			if m.cgroups.Rootless && sys.Name() == "devices" {
 				continue
 			}
-			// When m.rootless is true, errors from the device subsystem are ignored because it is really not expected to work.
 			// However, errors from other subsystems are not ignored.
 			// see @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error"
 			if path == "" {
@@ -196,7 +198,7 @@ func (m *manager) Set(r *configs.Resources) error {
 // provided
 func (m *manager) Freeze(state configs.FreezerState) error {
 	path := m.Path("freezer")
-	if m.cgroups == nil || path == "" {
+	if path == "" {
 		return errors.New("cannot toggle freezer: cgroups not configured for container")
 	}
 
@@ -249,7 +251,7 @@ func OOMKillCount(path string) (uint64, error) {
 func (m *manager) OOMKillCount() (uint64, error) {
 	c, err := OOMKillCount(m.Path("memory"))
 	// Ignore ENOENT when rootless as it couldn't create cgroup.
-	if err != nil && m.rootless && os.IsNotExist(err) {
+	if err != nil && m.cgroups.Rootless && os.IsNotExist(err) {
 		err = nil
 	}
 

libcontainer/cgroups/fs/fs_test.go

@@ -23,8 +23,11 @@ func BenchmarkGetStats(b *testing.B) {
 		Path:      "/some/kind/of/a/path/here",
 		Resources: &configs.Resources{},
 	}
-	m := NewManager(cg, nil, false)
-	err := m.Apply(-1)
+	m, err := NewManager(cg, nil)
+	if err != nil {
+		b.Fatal(err)
+	}
+	err = m.Apply(-1)
 	if err != nil {
 		b.Fatal(err)
 	}

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
 

libcontainer/cgroups/fs2/defaultpath.go

@@ -26,7 +26,7 @@ import (
 	"strings"
 
 	"github.com/opencontainers/runc/libcontainer/configs"
-	libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils"
+	"github.com/opencontainers/runc/libcontainer/utils"
 )
 
 const UnifiedMountpoint = "/sys/fs/cgroup"
@@ -36,20 +36,19 @@ func defaultDirPath(c *configs.Cgroup) (string, error) {
 		return "", fmt.Errorf("cgroup: either Path or Name and Parent should be used, got %+v", c)
 	}
 
-	// XXX: Do not remove this code. Path safety is important! -- cyphar
-	cgPath := libcontainerUtils.CleanPath(c.Path)
-	cgParent := libcontainerUtils.CleanPath(c.Parent)
-	cgName := libcontainerUtils.CleanPath(c.Name)
-
-	return _defaultDirPath(UnifiedMountpoint, cgPath, cgParent, cgName)
+	return _defaultDirPath(UnifiedMountpoint, c.Path, c.Parent, c.Name)
 }
 
 func _defaultDirPath(root, cgPath, cgParent, cgName string) (string, error) {
 	if (cgName != "" || cgParent != "") && cgPath != "" {
 		return "", errors.New("cgroup: either Path or Name and Parent should be used")
 	}
-	innerPath := cgPath
+
+	// XXX: Do not remove CleanPath. Path safety is important! -- cyphar
+	innerPath := utils.CleanPath(cgPath)
 	if innerPath == "" {
+		cgParent := utils.CleanPath(cgParent)
+		cgName := utils.CleanPath(cgName)
 		innerPath = filepath.Join(cgParent, cgName)
 	}
 	if filepath.IsAbs(innerPath) {

libcontainer/cgroups/fs2/fs2.go

@@ -20,16 +20,12 @@ type manager struct {
 	// controllers is content of "cgroup.controllers" file.
 	// excludes pseudo-controllers ("devices" and "freezer").
 	controllers map[string]struct{}
-	rootless    bool
 }
 
 // NewManager creates a manager for cgroup v2 unified hierarchy.
 // dirPath is like "/sys/fs/cgroup/user.slice/user-1001.slice/session-1.scope".
 // If dirPath is empty, it is automatically set using config.
-func NewManager(config *configs.Cgroup, dirPath string, rootless bool) (cgroups.Manager, error) {
-	if config == nil {
-		config = &configs.Cgroup{}
-	}
+func NewManager(config *configs.Cgroup, dirPath string) (cgroups.Manager, error) {
 	if dirPath == "" {
 		var err error
 		dirPath, err = defaultDirPath(config)
@@ -39,9 +35,8 @@ func NewManager(config *configs.Cgroup, dirPath string, rootless bool) (cgroups.
 	}
 
 	m := &manager{
-		config:   config,
-		dirPath:  dirPath,
-		rootless: rootless,
+		config:  config,
+		dirPath: dirPath,
 	}
 	return m, nil
 }
@@ -53,7 +48,7 @@ func (m *manager) getControllers() error {
 
 	data, err := cgroups.ReadFile(m.dirPath, "cgroup.controllers")
 	if err != nil {
-		if m.rootless && m.config.Path == "" {
+		if m.config.Rootless && m.config.Path == "" {
 			return nil
 		}
 		return err
@@ -73,7 +68,7 @@ func (m *manager) Apply(pid int) error {
 		// - "runc create (no limits + no cgrouppath + no permission) succeeds"
 		// - "runc create (rootless + no limits + cgrouppath + no permission) fails with permission error"
 		// - "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error"
-		if m.rootless {
+		if m.config.Rootless {
 			if m.config.Path == "" {
 				if blNeed, nErr := needAnyControllers(m.config.Resources); nErr == nil && !blNeed {
 					return nil
@@ -127,13 +122,16 @@ func (m *manager) GetStats() (*cgroups.Stats, error) {
 	if err := fscommon.RdmaGetStats(m.dirPath, st); err != nil && !os.IsNotExist(err) {
 		errs = append(errs, err)
 	}
-	if len(errs) > 0 && !m.rootless {
+	if len(errs) > 0 && !m.config.Rootless {
 		return st, fmt.Errorf("error while statting cgroup v2: %+v", errs)
 	}
 	return st, nil
 }
 
 func (m *manager) Freeze(state configs.FreezerState) error {
+	if m.config.Resources == nil {
+		return errors.New("cannot toggle freezer: cgroups not configured for container")
+	}
 	if err := setFreezer(m.dirPath, state); err != nil {
 		return err
 	}
@@ -150,6 +148,9 @@ func (m *manager) Path(_ string) string {
 }
 
 func (m *manager) Set(r *configs.Resources) error {
+	if r == nil {
+		return nil
+	}
 	if err := m.getControllers(); err != nil {
 		return err
 	}
@@ -171,10 +172,10 @@ func (m *manager) Set(r *configs.Resources) error {
 	}
 	// devices (since kernel 4.15, pseudo-controller)
 	//
-	// When m.rootless is true, errors from the device subsystem are ignored because it is really not expected to work.
+	// When rootless is true, errors from the device subsystem are ignored because it is really not expected to work.
 	// However, errors from other subsystems are not ignored.
 	// see @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error"
-	if err := setDevices(m.dirPath, r); err != nil && !m.rootless {
+	if err := setDevices(m.dirPath, r); err != nil && !m.config.Rootless {
 		return err
 	}
 	// cpuset (since kernel 5.0)
@@ -250,7 +251,7 @@ func OOMKillCount(path string) (uint64, error) {
 
 func (m *manager) OOMKillCount() (uint64, error) {
 	c, err := OOMKillCount(m.dirPath)
-	if err != nil && m.rootless && os.IsNotExist(err) {
+	if err != nil && m.config.Rootless && os.IsNotExist(err) {
 		err = nil
 	}
 

libcontainer/cgroups/manager/manager_test.go

@@ -0,0 +1,44 @@
+package manager
+
+import (
+	"testing"
+
+	"github.com/opencontainers/runc/libcontainer/configs"
+)
+
+// TestNilResources checks that a cgroup manager do not panic when
+// config.Resources is nil. While it does not make sense to use a
+// manager with no resources, it should not result in a panic.
+//
+// This tests either v1 or v2 managers (both fs and systemd),
+// depending on what cgroup version is available on the host.
+func TestNilResources(t *testing.T) {
+	for _, sd := range []bool{false, true} {
+		cg := &configs.Cgroup{} // .Resources is nil
+		cg.Systemd = sd
+		mgr, err := New(cg)
+		if err != nil {
+			// Some managers require non-nil Resources during
+			// instantiation -- provide and retry. In such case
+			// we're mostly testing Set(nil) below.
+			cg.Resources = &configs.Resources{}
+			mgr, err = New(cg)
+			if err != nil {
+				t.Error(err)
+				continue
+			}
+		}
+		_ = mgr.Apply(-1)
+		_ = mgr.Set(nil)
+		_ = mgr.Freeze(configs.Thawed)
+		_ = mgr.Exists()
+		_, _ = mgr.GetAllPids()
+		_, _ = mgr.GetCgroups()
+		_, _ = mgr.GetFreezerState()
+		_ = mgr.Path("")
+		_ = mgr.GetPaths()
+		_, _ = mgr.GetStats()
+		_, _ = mgr.OOMKillCount()
+		_ = mgr.Destroy()
+	}
+}