From e01db0028d1f9f8b5d5704c673b51ca6fa5cbf29 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 11 Nov 2024 23:12:32 -0800 Subject: [PATCH 1/4] runc delete: fix for rootless cgroup + ro cgroupfs An issue with runc 1.2.0 was reported to buildkit, in which runc delete returns with an error, with the log saying: > unable to destroy container: unable to remove container's cgroup: open /sys/fs/cgroup/snschvixiy3s74w74fjantrdg: no such file or directory Apparently, what happens is runc is running with no cgroup access (because /sys/fs/cgroup is mounted read-only). In this case error to create a cgroup path (in runc create/run) is ignored, but cgroup removal (in runc delete) is not. This is caused by commit d3d7f7d, which changes the cgroup removal logic in RemovePath. In the current code, if the initial rmdir has failed (in this case with EROFS), but the subsequent os.ReadDir returns ENOENT, it is returned (instead of being ignored -- as the path does not exist and so there is nothing to remove). Here is the minimal fix for the issue. Signed-off-by: Kir Kolyshkin (cherry picked from commit db59489b680104319541b0614c30229c8fa0270f) Signed-off-by: lfbzhm --- libcontainer/cgroups/utils.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index a05945cba6c..f699ed709a6 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -257,7 +257,10 @@ func RemovePath(path string) error { } infos, err := os.ReadDir(path) - if err != nil && !os.IsNotExist(err) { + if err != nil { + if os.IsNotExist(err) { + return nil + } return err } for _, info := range infos { From 79cdf119b62a576afb48e8d6f7e1a5a51a0a121a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 11 Nov 2024 23:13:27 -0800 Subject: [PATCH 2/4] libct/cg: RemovePath: simplify logic If the sub-cgroup RemovePath has failed for any reason, return the error right away. This way, we don't have to check for err != nil before retrying rmdir. This is a cosmetic change and should not change any functionality. Signed-off-by: Kir Kolyshkin (cherry picked from commit 12e06a7c4f081fc6fb4347741bf054f7ee7c256b) Signed-off-by: lfbzhm --- libcontainer/cgroups/utils.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index f699ed709a6..e3605856568 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -267,14 +267,11 @@ func RemovePath(path string) error { 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 + return rmdir(path, true) } // RemovePaths iterates over the provided paths removing them. From e41cc272b609321b160a8f7806a4e6d21d25b21a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 11 Nov 2024 23:17:42 -0800 Subject: [PATCH 3/4] libct/cg: RemovePath: improve comments Let's explain in greater details what's happening here and why. Signed-off-by: Kir Kolyshkin (cherry picked from commit ba3d026e5267c3b456ad946cc30877e303479f84) Signed-off-by: lfbzhm --- libcontainer/cgroups/utils.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index e3605856568..d404647c8c0 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -251,11 +251,21 @@ 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 { if os.IsNotExist(err) { @@ -263,14 +273,16 @@ func RemovePath(path string) error { } 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 { return err } } } + // Finally, try rmdir again, this time with retries on EBUSY, + // which may help with scenario 2 above. return rmdir(path, true) } From 832faf08fa3785669afa302b2911ba811efc7030 Mon Sep 17 00:00:00 2001 From: lfbzhm Date: Tue, 12 Nov 2024 14:15:41 +0000 Subject: [PATCH 4/4] libct/cg: add test for remove a non-existent dir in a ro mount point Signed-off-by: lfbzhm (cherry picked from commit 119111a0dfaece9a5ba4787fa49f5ae6efbd0b23) Signed-off-by: lfbzhm --- libcontainer/cgroups/utils_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) 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) + } +}