From d94a789d150bc0f736c8a16d5fa1932db0aea613 Mon Sep 17 00:00:00 2001 From: Marat Radchenko Date: Mon, 4 Sep 2023 12:39:47 +0300 Subject: [PATCH] Fix usages of `mountinfo.PrefixFilter` It says: The prefix path **must be absolute, have all symlinks resolved, and cleaned**. But those requirements are violated in lots of places. What happens when it is given a non-canonicalized path is that `mountinfo.GetMounts` will not find mounts. The trivial case is: ``` $ mkdir a && ln -s a b && mkdir b/c b/d && mount --bind b/c b/d && cat /proc/mounts | grep -- '[ab]/d' /dev/sdd3 /home/user/a/d ext4 rw,noatime,discard 0 0 ``` We asked to bind-mount b/c to b/d, but ended up with mount in a/d. So, mount table always contains canonicalized mount points, and it is an error to look for non-canonicalized paths in it. Signed-off-by: Marat Radchenko --- mount/lookup_unix.go | 7 ++----- mount/mount.go | 13 +++++++++++++ mount/mount_unix.go | 6 ++++++ mount/temp_unix.go | 12 +++++------- pkg/cri/sbserver/helpers_linux.go | 5 +++++ pkg/cri/sbserver/podsandbox/helpers_linux.go | 5 +++++ pkg/cri/server/helpers_linux.go | 5 +++++ 7 files changed, 41 insertions(+), 12 deletions(-) diff --git a/mount/lookup_unix.go b/mount/lookup_unix.go index 6755c0b22..90a0d5a5f 100644 --- a/mount/lookup_unix.go +++ b/mount/lookup_unix.go @@ -20,18 +20,15 @@ package mount import ( "fmt" - "path/filepath" "github.com/moby/sys/mountinfo" ) // Lookup returns the mount info corresponds to the path. func Lookup(dir string) (Info, error) { - dir = filepath.Clean(dir) - - resolvedDir, err := filepath.EvalSymlinks(dir) + resolvedDir, err := CanonicalizePath(dir) if err != nil { - return Info{}, fmt.Errorf("failed to resolve symlink for %q: %w", dir, err) + return Info{}, err } m, err := mountinfo.GetMounts(mountinfo.ParentsFilter(resolvedDir)) diff --git a/mount/mount.go b/mount/mount.go index defb1b944..16865b4f4 100644 --- a/mount/mount.go +++ b/mount/mount.go @@ -18,6 +18,7 @@ package mount import ( "fmt" + "path/filepath" "strings" "github.com/containerd/containerd/api/types" @@ -69,6 +70,18 @@ func UnmountMounts(mounts []Mount, target string, flags int) error { return nil } +// CanonicalizePath makes path absolute and resolves symlinks in it. +// Path must exist. +func CanonicalizePath(path string) (string, error) { + // Abs also does Clean, so we do not need to call it separately + path, err := filepath.Abs(path) + if err != nil { + return "", err + } + + return filepath.EvalSymlinks(path) +} + // ReadOnly returns a boolean value indicating whether this mount has the "ro" // option set. func (m *Mount) ReadOnly() bool { diff --git a/mount/mount_unix.go b/mount/mount_unix.go index 46dfd1c34..a1815956a 100644 --- a/mount/mount_unix.go +++ b/mount/mount_unix.go @@ -30,6 +30,12 @@ func UnmountRecursive(target string, flags int) error { if target == "" { return nil } + + target, err := CanonicalizePath(target) + if err != nil { + return err + } + mounts, err := mountinfo.GetMounts(mountinfo.PrefixFilter(target)) if err != nil { return err diff --git a/mount/temp_unix.go b/mount/temp_unix.go index 5ddd2cd16..8845e47aa 100644 --- a/mount/temp_unix.go +++ b/mount/temp_unix.go @@ -20,7 +20,6 @@ package mount import ( "os" - "path/filepath" "sort" "github.com/moby/sys/mountinfo" @@ -28,15 +27,13 @@ import ( // SetTempMountLocation sets the temporary mount location func SetTempMountLocation(root string) error { - root, err := filepath.Abs(root) + err := os.MkdirAll(root, 0700) if err != nil { return err } - if err := os.MkdirAll(root, 0700); err != nil { - return err - } - tempMountLocation = root - return nil + // We need to pass canonicalized path to mountinfo.PrefixFilter in CleanupTempMounts + tempMountLocation, err = CanonicalizePath(root) + return err } // CleanupTempMounts all temp mounts and remove the directories @@ -45,6 +42,7 @@ func CleanupTempMounts(flags int) (warnings []error, err error) { if err != nil { return nil, err } + // Make the deepest mount be first sort.Slice(mounts, func(i, j int) bool { return len(mounts[i].Mountpoint) > len(mounts[j].Mountpoint) diff --git a/pkg/cri/sbserver/helpers_linux.go b/pkg/cri/sbserver/helpers_linux.go index bd142ecc0..4214ced53 100644 --- a/pkg/cri/sbserver/helpers_linux.go +++ b/pkg/cri/sbserver/helpers_linux.go @@ -65,6 +65,11 @@ func openLogFile(path string) (*os.File, error) { // unmountRecursive unmounts the target and all mounts underneath, starting with // the deepest mount first. func unmountRecursive(ctx context.Context, target string) error { + target, err := mount.CanonicalizePath(target) + if err != nil { + return err + } + toUnmount, err := mountinfo.GetMounts(mountinfo.PrefixFilter(target)) if err != nil { return err diff --git a/pkg/cri/sbserver/podsandbox/helpers_linux.go b/pkg/cri/sbserver/podsandbox/helpers_linux.go index f51e9907e..84b5617a3 100644 --- a/pkg/cri/sbserver/podsandbox/helpers_linux.go +++ b/pkg/cri/sbserver/podsandbox/helpers_linux.go @@ -143,6 +143,11 @@ func (c *Controller) seccompEnabled() bool { // unmountRecursive unmounts the target and all mounts underneath, starting with // the deepest mount first. func unmountRecursive(ctx context.Context, target string) error { + target, err := mount.CanonicalizePath(target) + if err != nil { + return err + } + toUnmount, err := mountinfo.GetMounts(mountinfo.PrefixFilter(target)) if err != nil { return err diff --git a/pkg/cri/server/helpers_linux.go b/pkg/cri/server/helpers_linux.go index d490ce881..446ab3cb0 100644 --- a/pkg/cri/server/helpers_linux.go +++ b/pkg/cri/server/helpers_linux.go @@ -164,6 +164,11 @@ func openLogFile(path string) (*os.File, error) { // unmountRecursive unmounts the target and all mounts underneath, starting with // the deepest mount first. func unmountRecursive(ctx context.Context, target string) error { + target, err := mount.CanonicalizePath(target) + if err != nil { + return err + } + toUnmount, err := mountinfo.GetMounts(mountinfo.PrefixFilter(target)) if err != nil { return err