Merge pull request #4531 from lifubang/backport-4523

[1.2] runc delete: fix for rootless cgroup + ro cgroupfs

libcontainer/cgroups/utils.go

@@ -251,27 +251,39 @@ again:
 // RemovePath aims to remove cgroup path. It does so recursively,
 // by removing any subdirectories (sub-cgroups) first.
 func RemovePath(path string) error {
-	// Try the fast path first.
+	// Try the fast path first; don't retry on EBUSY yet.
 	if err := rmdir(path, false); err == nil {
 		return nil
 	}
 
+	// There are many reasons why rmdir can fail, including:
+	// 1. cgroup have existing sub-cgroups;
+	// 2. cgroup (still) have some processes (that are about to vanish);
+	// 3. lack of permission (one example is read-only /sys/fs/cgroup mount,
+	//    in which case rmdir returns EROFS even for for a non-existent path,
+	//    see issue 4518).
+	//
+	// Using os.ReadDir here kills two birds with one stone: check if
+	// the directory exists (handling scenario 3 above), and use
+	// directory contents to remove sub-cgroups (handling scenario 1).
 	infos, err := os.ReadDir(path)
-	if err != nil && !os.IsNotExist(err) {
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil
+		}
 		return err
 	}
+	// Let's remove sub-cgroups, if any.
 	for _, info := range infos {
 		if info.IsDir() {
-			// We should remove subcgroup first.
 			if err = RemovePath(filepath.Join(path, info.Name())); err != nil {
-				break
+				return err
 			}
 		}
 	}
-	if err == nil {
-		err = rmdir(path, true)
-	}
-	return err
+	// Finally, try rmdir again, this time with retries on EBUSY,
+	// which may help with scenario 2 above.
+	return rmdir(path, true)
 }
 
 // RemovePaths iterates over the provided paths removing them.

libcontainer/cgroups/utils_test.go

@@ -3,11 +3,13 @@ package cgroups
 import (
 	"bytes"
 	"errors"
+	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/moby/sys/mountinfo"
+	"golang.org/x/sys/unix"
 )
 
 const fedoraMountinfo = `15 35 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw
@@ -661,3 +663,29 @@ func TestConvertBlkIOToIOWeightValue(t *testing.T) {
 		}
 	}
 }
+
+// TestRemovePathReadOnly is to test remove a non-existent dir in a ro mount point.
+// The similar issue example: https://github.com/opencontainers/runc/issues/4518
+func TestRemovePathReadOnly(t *testing.T) {
+	dirTo := t.TempDir()
+	err := unix.Mount(t.TempDir(), dirTo, "", unix.MS_BIND, "")
+	if err != nil {
+		t.Skip("no permission of mount")
+	}
+	defer func() {
+		_ = unix.Unmount(dirTo, 0)
+	}()
+	err = unix.Mount("", dirTo, "", unix.MS_REMOUNT|unix.MS_BIND|unix.MS_RDONLY, "")
+	if err != nil {
+		t.Skip("no permission of mount")
+	}
+	nonExistentDir := filepath.Join(dirTo, "non-existent-dir")
+	err = rmdir(nonExistentDir, true)
+	if !errors.Is(err, unix.EROFS) {
+		t.Fatalf("expected the error of removing a non-existent dir %s in a ro mount point with rmdir to be unix.EROFS, but got: %v", nonExistentDir, err)
+	}
+	err = RemovePath(nonExistentDir)
+	if err != nil {
+		t.Fatalf("expected the error of removing a non-existent dir %s in a ro mount point with RemovePath to be nil, but got: %v", nonExistentDir, err)
+	}
+}