From 004f3951d5bfbbc2ba30536b97ab519679fcd679 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Mon, 23 Sep 2024 16:13:08 +0200 Subject: [PATCH 1/3] core/mount: Use MNT_DETACH for umount of tmp layers Overlayfs needs to do an idmap mount of each layer and the cleanup function just unmounts and deletes the directories. However, when the resource is busy, the umount fails. Let's make the unmount detached so the unmount will eventually be done when it's not busy anymore. Also, making it detached solves the issues with the unmount failing because it is busy. Big kudos to @mbaynton for reporting this issue with lot of details, nailing it down to containerd lines of code and showing all the log lines to understand the big picture. Fixes: #10704 Signed-off-by: Rodrigo Campos --- core/mount/mount_linux.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/mount/mount_linux.go b/core/mount/mount_linux.go index 330760335..ec5417ed7 100644 --- a/core/mount/mount_linux.go +++ b/core/mount/mount_linux.go @@ -251,7 +251,9 @@ func doPrepareIDMappedOverlay(lowerDirs []string, usernsFd int) (tmpLowerDirs [] } cleanUp := func() { for _, lowerDir := range tmpLowerDirs { - if err := unix.Unmount(lowerDir, 0); err != nil { + // Do a detached unmount so even if the resource is busy, the mount will be + // gone (eventually) and we can safely delete the directory too. + if err := unix.Unmount(lowerDir, unix.MNT_DETACH); err != nil { log.L.WithError(err).Warnf("failed to unmount temp lowerdir %s", lowerDir) } } From f8d84ecf92b0954e53115b015e68307664066602 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Mon, 23 Sep 2024 16:15:10 +0200 Subject: [PATCH 2/3] core/mount: Prevent accidental removal of rootfs files Using os.RemoveAll() is quite risky, as if the unmount failed and we can delete files from the container rootfs. In fact, we were doing just that. Let's use os.Remove() to make sure we only deleted empty dirs. Big kudos to @mbaynton for reporting this issue with lot of details, nailing it down to containerd lines of code and showing all the log lines to understand the big picture. Fixes: #10704 Signed-off-by: Rodrigo Campos --- core/mount/mount_linux.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/core/mount/mount_linux.go b/core/mount/mount_linux.go index ec5417ed7..cd3b354cf 100644 --- a/core/mount/mount_linux.go +++ b/core/mount/mount_linux.go @@ -256,9 +256,16 @@ func doPrepareIDMappedOverlay(lowerDirs []string, usernsFd int) (tmpLowerDirs [] if err := unix.Unmount(lowerDir, unix.MNT_DETACH); err != nil { log.L.WithError(err).Warnf("failed to unmount temp lowerdir %s", lowerDir) } + // Using os.Remove() so if it's not empty, we don't delete files in the + // rootfs. + if err := os.Remove(lowerDir); err != nil { + log.L.WithError(err).Warnf("failed to remove temporary overlay lowerdir's") + } } - if terr := os.RemoveAll(filepath.Clean(filepath.Join(tmpLowerDirs[0], ".."))); terr != nil { - log.L.WithError(terr).Warnf("failed to remove temporary overlay lowerdir's") + + // This dir should be empty now. Otherwise, we don't do anything. + if err := os.Remove(filepath.Join(tmpLowerDirs[0], "..")); err != nil { + log.L.WithError(err).Infof("failed to remove temporary overlay dir") } } for i, lowerDir := range lowerDirs { From 30f28933513e62a3243b88420e00af0e1b912b13 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Tue, 24 Sep 2024 17:44:52 +0200 Subject: [PATCH 3/3] core/mount: Only remove dirs if unmount succeeded The detached mount is less likely to fail in our case, but if we see any failure to unmount, we should just skip the removal of directories. Signed-off-by: Rodrigo Campos --- core/mount/mount_linux.go | 1 + 1 file changed, 1 insertion(+) diff --git a/core/mount/mount_linux.go b/core/mount/mount_linux.go index cd3b354cf..a14d545fd 100644 --- a/core/mount/mount_linux.go +++ b/core/mount/mount_linux.go @@ -255,6 +255,7 @@ func doPrepareIDMappedOverlay(lowerDirs []string, usernsFd int) (tmpLowerDirs [] // gone (eventually) and we can safely delete the directory too. if err := unix.Unmount(lowerDir, unix.MNT_DETACH); err != nil { log.L.WithError(err).Warnf("failed to unmount temp lowerdir %s", lowerDir) + continue } // Using os.Remove() so if it's not empty, we don't delete files in the // rootfs.