From dd7fe0b76f84772002c2fa10e089455bdf71c6c2 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Wed, 16 Jul 2025 16:53:37 +0200 Subject: [PATCH] core/mount: Properly cleanup on doPrepareIDMappedOverlay errors Before this patch, the cleanup was unconditionally trying an unmount, which is not needed if the mount never succeeded in the first place. This causes logs of failed stuff, that should never be there. Also, one function was creating the tmpDir and the nested one removing it (in the cleanup function), which complicates the reasoning about not leaking resources. This patch on one hand moves the creation of the tmpDir into the function that will remove it later and renames the parameter. On the other hand, it also separates the cleanup function in two functions: cleanDir() and cleanMount(). Each one will clean the directory or the mount, only if needed, and a new cleanup function that just calls cleanDir() and cleanMount() is returned as the cleanup function handler. Because we now create the tmp directory inside the function, we need to adjust the test: the directory passed as param now will exist, but it shoild be empty. Therefore, we change the os.Stat() to an os.Remove(). If it's not empty (the cleanup didn't work), the remove will fail. Signed-off-by: Rodrigo Campos --- core/mount/mount_linux.go | 37 +++++++++++++++++++++------------- core/mount/mount_linux_test.go | 9 +++------ 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/core/mount/mount_linux.go b/core/mount/mount_linux.go index 43a1d1ba61..930814cfab 100644 --- a/core/mount/mount_linux.go +++ b/core/mount/mount_linux.go @@ -62,12 +62,7 @@ func prepareIDMappedOverlay(usernsFd int, options []string) ([]string, func(), e return options, nil, fmt.Errorf("failed to parse overlay lowerdir's from given options") } - tempRemountsLocation, err := os.MkdirTemp(tempMountLocation, "ovl-idmapped") - if err != nil { - return options, nil, fmt.Errorf("failed to create temporary overlay lowerdir mount location: %w", err) - } - - tmpLowerdirs, idMapCleanUp, err := doPrepareIDMappedOverlay(tempRemountsLocation, lowerDirs, usernsFd) + tmpLowerdirs, idMapCleanUp, err := doPrepareIDMappedOverlay(tempMountLocation, lowerDirs, usernsFd) if err != nil { return options, idMapCleanUp, fmt.Errorf("failed to create idmapped mount: %w", err) } @@ -245,24 +240,24 @@ func getUnprivilegedMountFlags(path string) (int, error) { return flags, nil } -func doPrepareIDMappedOverlay(tempRemountsLocation string, lowerDirs []string, usernsFd int) (tmpLowerDirs []string, cleanup func(), retErr error) { +func doPrepareIDMappedOverlay(tmpDir string, lowerDirs []string, usernsFd int) (_ []string, _ func(), retErr error) { commonDir, err := getCommonDirectory(lowerDirs) if err != nil { return nil, nil, fmt.Errorf("failed to determine common parent: %w", err) } - cleanup = func() { - if err := unix.Unmount(tempRemountsLocation, 0); err != nil { - log.L.WithError(err).Warnf("failed to unmount idmapped directory %s", tempRemountsLocation) - } - // Using os.Remove() so if it's not empty, we don't delete files in the rootfs. + tempRemountsLocation, err := os.MkdirTemp(tmpDir, "ovl-idmapped") + if err != nil { + return nil, nil, fmt.Errorf("failed to create temporary overlay lowerdir mount location: %w", err) + } + cleanDir := func() { if err := os.Remove(tempRemountsLocation); err != nil { log.L.WithError(err).Infof("failed to remove idmapped directory") } } defer func() { if retErr != nil { - cleanup() + cleanDir() } }() @@ -270,10 +265,24 @@ func doPrepareIDMappedOverlay(tempRemountsLocation string, lowerDirs []string, u if err := IDMapMountWithAttrs(commonDir, tempRemountsLocation, usernsFd, unix.MOUNT_ATTR_RDONLY, 0); err != nil { return nil, nil, err } + cleanMount := func() { + if err := unix.Unmount(tempRemountsLocation, 0); err != nil { + log.L.WithError(err).Warnf("failed to unmount idmapped directory %s", tempRemountsLocation) + } + } + defer func() { + if retErr != nil { + cleanMount() + } + }() // Build new lower dir paths through the idmapped directory - tmpLowerDirs = buildIDMappedPaths(lowerDirs, commonDir, tempRemountsLocation) + tmpLowerDirs := buildIDMappedPaths(lowerDirs, commonDir, tempRemountsLocation) + cleanup := func() { + cleanMount() + cleanDir() + } return tmpLowerDirs, cleanup, nil } diff --git a/core/mount/mount_linux_test.go b/core/mount/mount_linux_test.go index f59f6cfa78..5021c81835 100644 --- a/core/mount/mount_linux_test.go +++ b/core/mount/mount_linux_test.go @@ -18,7 +18,6 @@ package mount import ( "fmt" - "io/fs" "os" "os/exec" "path/filepath" @@ -279,15 +278,13 @@ func TestDoPrepareIDMappedOverlay(t *testing.T) { cleanup() - _, err = os.Stat(remountsLocation) + err = os.Remove(remountsLocation) if tc.injectUmountFault { // We should have failed to remove the remounts location if the unmount failed. - assert.NoError(t, err, "expected remounts location to still exist after unmount failure") + assert.Error(t, err, "expected remove to fail (dir not empty), expected remount child locations to still exist after unmount failure") } else { - pathErr, isPathErr := err.(*fs.PathError) - require.True(t, isPathErr, "expected a PathError") - assert.Equal(t, unix.ENOENT, pathErr.Err, "temporary remounts should be cleaned up") + assert.NoError(t, err, "expected remove to work (dir empty), the child directory should be unmounted and removed") } // Original lowerdirs should be unaffected.