diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index a05945cba6c..d404647c8c0 100644 --- a/libcontainer/cgroups/utils.go +++ b/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. diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go index fc81992f0b0..58ac85a5864 100644 --- a/libcontainer/cgroups/utils_test.go +++ b/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) + } +}