Make cgroup creation/deletion more robust

- Don't attempt to create directory is controller is not
  present in the system
- Ensure that all files being written exist in cgroupfs
- Attempt to delete directories during Uninstall even if
  other deletions have failed

Fixes #6446

PiperOrigin-RevId: 402614820

3 files changed, 93 insertions, 35 deletions

diff --git a/runsc/cgroup/BUILD b/runsc/cgroup/BUILD
index f7e892584..d3aec1fff 100644
--- a/runsc/cgroup/BUILD
+++ b/runsc/cgroup/BUILD
@@ -9,6 +9,7 @@ go_library(
     deps = [
         "//pkg/cleanup",
         "//pkg/log",
+        "//pkg/sync",
         "@com_github_cenkalti_backoff//:go_default_library",
         "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
         "@org_golang_x_sys//unix:go_default_library",
diff --git a/runsc/cgroup/cgroup.go b/runsc/cgroup/cgroup.go
index 7280a52fc..7a0f0694f 100644
--- a/runsc/cgroup/cgroup.go
+++ b/runsc/cgroup/cgroup.go
@@ -19,7 +19,6 @@ package cgroup
 import (
 	"bufio"
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -34,6 +33,7 @@ import (
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/log"
+	"gvisor.dev/gvisor/pkg/sync"
 )
 
 const (
@@ -104,17 +104,21 @@ func setOptionalValueUint16(path, name string, val *uint16) error {
 
 func setValue(path, name, data string) error {
 	fullpath := filepath.Join(path, name)
+	log.Debugf("Setting %q to %q", fullpath, data)
+	return writeFile(fullpath, []byte(data), 0700)
+}
 
-	// Retry writes on EINTR; see:
-	//    https://github.com/golang/go/issues/38033
-	for {
-		err := ioutil.WriteFile(fullpath, []byte(data), 0700)
-		if err == nil {
-			return nil
-		} else if !errors.Is(err, unix.EINTR) {
-			return err
-		}
+// writeFile is similar to ioutil.WriteFile() but doesn't create the file if it
+// doesn't exist.
+func writeFile(path string, data []byte, perm os.FileMode) error {
+	f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, perm)
+	if err != nil {
+		return err
 	}
+	defer f.Close()
+
+	_, err = f.Write(data)
+	return err
 }
 
 func getValue(path, name string) (string, error) {
@@ -155,15 +159,8 @@ func fillFromAncestor(path string) (string, error) {
 		return "", err
 	}
 
-	// Retry writes on EINTR; see:
-	//    https://github.com/golang/go/issues/38033
-	for {
-		err := ioutil.WriteFile(path, []byte(val), 0700)
-		if err == nil {
-			break
-		} else if !errors.Is(err, unix.EINTR) {
-			return "", err
-		}
+	if err := writeFile(path, []byte(val), 0700); err != nil {
+		return "", nil
 	}
 	return val, nil
 }
@@ -371,21 +368,20 @@ func (c *Cgroup) Install(res *specs.LinuxResources) error {
 	}
 	for _, key := range missing {
 		ctrlr := controllers[key]
-		path := c.MakePath(key)
-		log.Debugf("Creating cgroup %q: %q", key, path)
-		if err := os.MkdirAll(path, 0755); err != nil {
-			if ctrlr.optional() && errors.Is(err, unix.EROFS) {
-				if err := ctrlr.skip(res); err != nil {
-					return err
-				}
-				log.Infof("Skipping cgroup %q", key)
-				continue
+
+		if skip, err := c.createController(key); skip && ctrlr.optional() {
+			if err := ctrlr.skip(res); err != nil {
+				return err
 			}
+			log.Infof("Skipping cgroup %q, err: %v", key, err)
+			continue
+		} else if err != nil {
 			return err
 		}
 
 		// Only set controllers that were created by me.
 		c.Own[key] = true
+		path := c.MakePath(key)
 		if err := ctrlr.set(res, path); err != nil {
 			return err
 		}
@@ -394,10 +390,29 @@ func (c *Cgroup) Install(res *specs.LinuxResources) error {
 	return nil
 }
 
+// createController creates the controller directory, checking that the
+// controller is enabled in the system. It returns a boolean indicating whether
+// the controller should be skipped (e.g. controller is disabled). In case it
+// should be skipped, it also returns the error it got.
+func (c *Cgroup) createController(name string) (bool, error) {
+	ctrlrPath := filepath.Join(cgroupRoot, name)
+	if _, err := os.Stat(ctrlrPath); err != nil {
+		return os.IsNotExist(err), err
+	}
+
+	path := c.MakePath(name)
+	log.Debugf("Creating cgroup %q: %q", name, path)
+	if err := os.MkdirAll(path, 0755); err != nil {
+		return false, err
+	}
+	return false, nil
+}
+
 // Uninstall removes the settings done in Install(). If cgroup path already
 // existed when Install() was called, Uninstall is a noop.
 func (c *Cgroup) Uninstall() error {
 	log.Debugf("Deleting cgroup %q", c.Name)
+	wait := sync.WaitGroupErr{}
 	for key := range controllers {
 		if !c.Own[key] {
 			// cgroup is managed by caller, don't touch it.
@@ -406,9 +421,8 @@ func (c *Cgroup) Uninstall() error {
 		path := c.MakePath(key)
 		log.Debugf("Removing cgroup controller for key=%q path=%q", key, path)
 
-		// If we try to remove the cgroup too soon after killing the
-		// sandbox we might get EBUSY, so we retry for a few seconds
-		// until it succeeds.
+		// If we try to remove the cgroup too soon after killing the sandbox we
+		// might get EBUSY, so we retry for a few seconds until it succeeds.
 		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 		defer cancel()
 		b := backoff.WithContext(backoff.NewConstantBackOff(100*time.Millisecond), ctx)
@@ -419,11 +433,18 @@ func (c *Cgroup) Uninstall() error {
 			}
 			return err
 		}
-		if err := backoff.Retry(fn, b); err != nil {
-			return fmt.Errorf("removing cgroup path %q: %w", path, err)
-		}
+		// Run deletions in parallel to remove all directories even if there are
+		// failures/timeouts in other directories.
+		wait.Add(1)
+		go func() {
+			defer wait.Done()
+			if err := backoff.Retry(fn, b); err != nil {
+				wait.ReportError(fmt.Errorf("removing cgroup path %q: %w", path, err))
+				return
+			}
+		}()
 	}
-	return nil
+	return wait.Error()
 }
 
 // Join adds the current process to the all controllers. Returns function that
diff --git a/runsc/cgroup/cgroup_test.go b/runsc/cgroup/cgroup_test.go
index 1431b4e8f..0b6a5431b 100644
--- a/runsc/cgroup/cgroup_test.go
+++ b/runsc/cgroup/cgroup_test.go
@@ -129,6 +129,18 @@ func boolPtr(v bool) *bool {
 	return &v
 }
 
+func createDir(dir string, contents map[string]string) error {
+	for name := range contents {
+		path := filepath.Join(dir, name)
+		f, err := os.Create(path)
+		if err != nil {
+			return err
+		}
+		f.Close()
+	}
+	return nil
+}
+
 func checkDir(t *testing.T, dir string, contents map[string]string) {
 	all, err := ioutil.ReadDir(dir)
 	if err != nil {
@@ -254,6 +266,9 @@ func TestBlockIO(t *testing.T) {
 				t.Fatalf("error creating temporary directory: %v", err)
 			}
 			defer os.RemoveAll(dir)
+			if err := createDir(dir, tc.wants); err != nil {
+				t.Fatalf("createDir(): %v", err)
+			}
 
 			spec := &specs.LinuxResources{
 				BlockIO: tc.spec,
@@ -304,6 +319,9 @@ func TestCPU(t *testing.T) {
 				t.Fatalf("error creating temporary directory: %v", err)
 			}
 			defer os.RemoveAll(dir)
+			if err := createDir(dir, tc.wants); err != nil {
+				t.Fatalf("createDir(): %v", err)
+			}
 
 			spec := &specs.LinuxResources{
 				CPU: tc.spec,
@@ -343,6 +361,9 @@ func TestCPUSet(t *testing.T) {
 				t.Fatalf("error creating temporary directory: %v", err)
 			}
 			defer os.RemoveAll(dir)
+			if err := createDir(dir, tc.wants); err != nil {
+				t.Fatalf("createDir(): %v", err)
+			}
 
 			spec := &specs.LinuxResources{
 				CPU: tc.spec,
@@ -481,6 +502,9 @@ func TestHugeTlb(t *testing.T) {
 				t.Fatalf("error creating temporary directory: %v", err)
 			}
 			defer os.RemoveAll(dir)
+			if err := createDir(dir, tc.wants); err != nil {
+				t.Fatalf("createDir(): %v", err)
+			}
 
 			spec := &specs.LinuxResources{
 				HugepageLimits: tc.spec,
@@ -542,6 +566,9 @@ func TestMemory(t *testing.T) {
 				t.Fatalf("error creating temporary directory: %v", err)
 			}
 			defer os.RemoveAll(dir)
+			if err := createDir(dir, tc.wants); err != nil {
+				t.Fatalf("createDir(): %v", err)
+			}
 
 			spec := &specs.LinuxResources{
 				Memory: tc.spec,
@@ -584,6 +611,9 @@ func TestNetworkClass(t *testing.T) {
 				t.Fatalf("error creating temporary directory: %v", err)
 			}
 			defer os.RemoveAll(dir)
+			if err := createDir(dir, tc.wants); err != nil {
+				t.Fatalf("createDir(): %v", err)
+			}
 
 			spec := &specs.LinuxResources{
 				Network: tc.spec,
@@ -631,6 +661,9 @@ func TestNetworkPriority(t *testing.T) {
 				t.Fatalf("error creating temporary directory: %v", err)
 			}
 			defer os.RemoveAll(dir)
+			if err := createDir(dir, tc.wants); err != nil {
+				t.Fatalf("createDir(): %v", err)
+			}
 
 			spec := &specs.LinuxResources{
 				Network: tc.spec,
@@ -671,6 +704,9 @@ func TestPids(t *testing.T) {
 				t.Fatalf("error creating temporary directory: %v", err)
 			}
 			defer os.RemoveAll(dir)
+			if err := createDir(dir, tc.wants); err != nil {
+				t.Fatalf("createDir(): %v", err)
+			}
 
 			spec := &specs.LinuxResources{
 				Pids: tc.spec,