From dbb6927ec229b5348f8712812a871ed9331eb0b5 Mon Sep 17 00:00:00 2001 From: j4ckstraw Date: Wed, 16 Aug 2023 17:10:34 +0800 Subject: [PATCH 1/5] Replace stat syscall with statx statx with AT_STATX_DONT_SYNC will not stuck kubelet syncLoop. Signed-off-by: j4ckstraw fix remove useless comment Signed-off-by: j4ckstraw --- staging/src/k8s.io/mount-utils/mount_linux.go | 69 +++++++++++++------ 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index 7d180723047..bb7e5e768b4 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -29,10 +29,10 @@ import ( "path/filepath" "strconv" "strings" - "syscall" "time" "github.com/moby/sys/mountinfo" + "golang.org/x/sys/unix" "k8s.io/klog/v2" utilexec "k8s.io/utils/exec" @@ -385,28 +385,55 @@ func (*Mounter) List() ([]MountPoint, error) { return ListProcMounts(procMountsPath) } -// IsLikelyNotMountPoint determines if a directory is not a mountpoint. -// It is fast but not necessarily ALWAYS correct. If the path is in fact -// a bind mount from one part of a mount to another it will not be detected. -// It also can not distinguish between mountpoints and symbolic links. -// mkdir /tmp/a /tmp/b; mount --bind /tmp/a /tmp/b; IsLikelyNotMountPoint("/tmp/b") -// will return true. When in fact /tmp/b is a mount point. If this situation -// is of interest to you, don't use this function... -func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { - stat, err := os.Stat(file) - if err != nil { - return true, err - } - rootStat, err := os.Stat(filepath.Dir(strings.TrimSuffix(file, "/"))) - if err != nil { - return true, err - } - // If the directory has a different device as parent, then it is a mountpoint. - if stat.Sys().(*syscall.Stat_t).Dev != rootStat.Sys().(*syscall.Stat_t).Dev { - return false, nil +// statx support since Linux 4.11, glibc 2.28 +// refer: https://man7.org/linux/man-pages/man2/statx.2.html +var errNotSupport = errors.New("The statx syscall is not supported. At least Linux kernel 4.11 is needed") + +func statx(file string) (unix.Statx_t, error) { + var stat unix.Statx_t + if err := unix.Statx(0, file, unix.AT_STATX_DONT_SYNC, 0, &stat); err != nil { + if err == unix.ENOSYS { + return stat, errNotSupport + } + + return stat, err } - return true, nil + return stat, nil +} + +// IsLikelyNotMountPoint determines if a directory is not a mountpoint. +// If fast check failed, fall back to slow path. If the path is in fact +// a bind mount from one part of a mount to another it will be detected. +// It also can distinguish between mountpoints and symbolic links. +// mkdir /tmp/a /tmp/b; mount --bind /tmp/a /tmp/b; IsLikelyNotMountPoint("/tmp/b") +// will return false. When in fact /tmp/b is a mount point. + +// TODO(j4ckstraw) add test +func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { + var stat, rootStat unix.Statx_t + var err error + + if stat, err = statx(file); err != nil { + if errors.Is(err, errNotSupport) { + // not support statx, go slow path + mnt, mntErr := mounter.IsMountPoint(file) + return !mnt, mntErr + } + + return false, err + } + + root := filepath.Dir(strings.TrimSuffix(file, "/")) + if rootStat, err = statx(root); err != nil { + return false, err + } + + // TODO add STATX_ATTR_MOUNT_ROOT support, which can check mountpoint correctly. + // Linux 5.8 commit 80340fe3605c0e78cfe496c3b3878be828cfdbfe + // stat->attributes |= STATX_ATTR_MOUNT_ROOT; + // stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT; + return !(stat.Dev_major == rootStat.Dev_major && stat.Dev_minor == rootStat.Dev_minor), nil } // CanSafelySkipMountPointCheck relies on the detected behavior of umount when given a target that is not a mount point. From 81ef146e2feb70645059c1d9cd9df43829f65ff7 Mon Sep 17 00:00:00 2001 From: j4ckstraw Date: Thu, 17 Aug 2023 09:58:08 +0800 Subject: [PATCH 2/5] feat: support statx STATX_ATTR_MOUNT_ROOT attribute Signed-off-by: j4ckstraw fix: not use glibc Signed-off-by: j4ckstraw fix linter error --- staging/src/k8s.io/mount-utils/mount_linux.go | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index bb7e5e768b4..89e7d0675ff 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -55,6 +55,11 @@ const ( errNotMounted = "not mounted" ) +var ( + // Error statx support since Linux 4.11, https://man7.org/linux/man-pages/man2/statx.2.html + errStatxNotSupport = errors.New("the statx syscall is not supported. At least Linux kernel 4.11 is needed") +) + // Mounter provides the default implementation of mount.Interface // for the linux platform. This implementation assumes that the // kubelet is running in the host's root mount namespace. @@ -385,15 +390,11 @@ func (*Mounter) List() ([]MountPoint, error) { return ListProcMounts(procMountsPath) } -// statx support since Linux 4.11, glibc 2.28 -// refer: https://man7.org/linux/man-pages/man2/statx.2.html -var errNotSupport = errors.New("The statx syscall is not supported. At least Linux kernel 4.11 is needed") - func statx(file string) (unix.Statx_t, error) { var stat unix.Statx_t if err := unix.Statx(0, file, unix.AT_STATX_DONT_SYNC, 0, &stat); err != nil { if err == unix.ENOSYS { - return stat, errNotSupport + return stat, errStatxNotSupport } return stat, err @@ -408,14 +409,12 @@ func statx(file string) (unix.Statx_t, error) { // It also can distinguish between mountpoints and symbolic links. // mkdir /tmp/a /tmp/b; mount --bind /tmp/a /tmp/b; IsLikelyNotMountPoint("/tmp/b") // will return false. When in fact /tmp/b is a mount point. - -// TODO(j4ckstraw) add test func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { var stat, rootStat unix.Statx_t var err error if stat, err = statx(file); err != nil { - if errors.Is(err, errNotSupport) { + if errors.Is(err, errStatxNotSupport) { // not support statx, go slow path mnt, mntErr := mounter.IsMountPoint(file) return !mnt, mntErr @@ -424,15 +423,23 @@ func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { return false, err } + if stat.Attributes_mask != 0 { + if stat.Attributes_mask&unix.STATX_ATTR_MOUNT_ROOT != 0 { + if stat.Attributes&unix.STATX_ATTR_MOUNT_ROOT != 0 { + // file is a mountpoint + return false, nil + } else { + // no need to check rootStat if unix.STATX_ATTR_MOUNT_ROOT supported + return true, nil + } + } + } + root := filepath.Dir(strings.TrimSuffix(file, "/")) if rootStat, err = statx(root); err != nil { return false, err } - // TODO add STATX_ATTR_MOUNT_ROOT support, which can check mountpoint correctly. - // Linux 5.8 commit 80340fe3605c0e78cfe496c3b3878be828cfdbfe - // stat->attributes |= STATX_ATTR_MOUNT_ROOT; - // stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT; return !(stat.Dev_major == rootStat.Dev_major && stat.Dev_minor == rootStat.Dev_minor), nil } From 47b1002bab47ffee51f2d25679cf9fa1932d5361 Mon Sep 17 00:00:00 2001 From: j4ckstraw Date: Wed, 30 Aug 2023 09:46:50 +0800 Subject: [PATCH 3/5] fallback to origin IsLikelyNotMountPoint to avoid regression Signed-off-by: j4ckstraw fix compile error --- staging/src/k8s.io/mount-utils/mount_linux.go | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index 89e7d0675ff..c2d1ab72bcf 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -29,6 +29,7 @@ import ( "path/filepath" "strconv" "strings" + "syscall" "time" "github.com/moby/sys/mountinfo" @@ -403,12 +404,30 @@ func statx(file string) (unix.Statx_t, error) { return stat, nil } +func (mounter *Mounter) isLikelyNotMountPoint(file string) (bool, error) { + stat, err := os.Stat(file) + if err != nil { + return true, err + } + rootStat, err := os.Stat(filepath.Dir(strings.TrimSuffix(file, "/"))) + if err != nil { + return true, err + } + // If the directory has a different device as parent, then it is a mountpoint. + if stat.Sys().(*syscall.Stat_t).Dev != rootStat.Sys().(*syscall.Stat_t).Dev { + return false, nil + } + + return true, nil +} + // IsLikelyNotMountPoint determines if a directory is not a mountpoint. -// If fast check failed, fall back to slow path. If the path is in fact -// a bind mount from one part of a mount to another it will be detected. -// It also can distinguish between mountpoints and symbolic links. +// It is fast but not necessarily ALWAYS correct. If the path is in fact +// a bind mount from one part of a mount to another it will not be detected. +// It also can not distinguish between mountpoints and symbolic links. // mkdir /tmp/a /tmp/b; mount --bind /tmp/a /tmp/b; IsLikelyNotMountPoint("/tmp/b") -// will return false. When in fact /tmp/b is a mount point. +// will return true. When in fact /tmp/b is a mount point. If this situation +// is of interest to you, don't use this function... func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { var stat, rootStat unix.Statx_t var err error @@ -416,8 +435,11 @@ func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { if stat, err = statx(file); err != nil { if errors.Is(err, errStatxNotSupport) { // not support statx, go slow path - mnt, mntErr := mounter.IsMountPoint(file) - return !mnt, mntErr + // mnt, mntErr := mounter.IsMountPoint(file) + // return !mnt, mntErr + + // fall back to isLikelyNotMountPoint + return mounter.isLikelyNotMountPoint(file) } return false, err From c3775de7479b4cb8048d56040364b4f7b4e3b425 Mon Sep 17 00:00:00 2001 From: j4ckstraw Date: Mon, 1 Jan 2024 18:06:24 +0800 Subject: [PATCH 4/5] fix logic error Signed-off-by: j4ckstraw --- staging/src/k8s.io/mount-utils/mount_linux.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index c2d1ab72bcf..2e9af12e49a 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -442,7 +442,7 @@ func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { return mounter.isLikelyNotMountPoint(file) } - return false, err + return true, err } if stat.Attributes_mask != 0 { @@ -459,10 +459,10 @@ func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { root := filepath.Dir(strings.TrimSuffix(file, "/")) if rootStat, err = statx(root); err != nil { - return false, err + return true, err } - return !(stat.Dev_major == rootStat.Dev_major && stat.Dev_minor == rootStat.Dev_minor), nil + return (stat.Dev_major == rootStat.Dev_major && stat.Dev_minor == rootStat.Dev_minor), nil } // CanSafelySkipMountPointCheck relies on the detected behavior of umount when given a target that is not a mount point. From d5664276bf5baf7ebf997609051dad240d09e1f5 Mon Sep 17 00:00:00 2001 From: j4ckstraw Date: Mon, 8 Jan 2024 09:46:37 +0800 Subject: [PATCH 5/5] refact IsLikelyNotMountPoint Signed-off-by: j4ckstraw --- staging/src/k8s.io/mount-utils/mount_linux.go | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index 2e9af12e49a..07ce76de198 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -404,7 +404,7 @@ func statx(file string) (unix.Statx_t, error) { return stat, nil } -func (mounter *Mounter) isLikelyNotMountPoint(file string) (bool, error) { +func (mounter *Mounter) isLikelyNotMountPointStat(file string) (bool, error) { stat, err := os.Stat(file) if err != nil { return true, err @@ -421,27 +421,11 @@ func (mounter *Mounter) isLikelyNotMountPoint(file string) (bool, error) { return true, nil } -// IsLikelyNotMountPoint determines if a directory is not a mountpoint. -// It is fast but not necessarily ALWAYS correct. If the path is in fact -// a bind mount from one part of a mount to another it will not be detected. -// It also can not distinguish between mountpoints and symbolic links. -// mkdir /tmp/a /tmp/b; mount --bind /tmp/a /tmp/b; IsLikelyNotMountPoint("/tmp/b") -// will return true. When in fact /tmp/b is a mount point. If this situation -// is of interest to you, don't use this function... -func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { +func (mounter *Mounter) isLikelyNotMountPointStatx(file string) (bool, error) { var stat, rootStat unix.Statx_t var err error if stat, err = statx(file); err != nil { - if errors.Is(err, errStatxNotSupport) { - // not support statx, go slow path - // mnt, mntErr := mounter.IsMountPoint(file) - // return !mnt, mntErr - - // fall back to isLikelyNotMountPoint - return mounter.isLikelyNotMountPoint(file) - } - return true, err } @@ -465,6 +449,23 @@ func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { return (stat.Dev_major == rootStat.Dev_major && stat.Dev_minor == rootStat.Dev_minor), nil } +// IsLikelyNotMountPoint determines if a directory is not a mountpoint. +// It is fast but not necessarily ALWAYS correct. If the path is in fact +// a bind mount from one part of a mount to another it will not be detected. +// It also can not distinguish between mountpoints and symbolic links. +// mkdir /tmp/a /tmp/b; mount --bind /tmp/a /tmp/b; IsLikelyNotMountPoint("/tmp/b") +// will return true. When in fact /tmp/b is a mount point. If this situation +// is of interest to you, don't use this function... +func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { + notMountPoint, err := mounter.isLikelyNotMountPointStatx(file) + if errors.Is(err, errStatxNotSupport) { + // fall back to isLikelyNotMountPointStat + return mounter.isLikelyNotMountPointStat(file) + } + + return notMountPoint, err +} + // CanSafelySkipMountPointCheck relies on the detected behavior of umount when given a target that is not a mount point. func (mounter *Mounter) CanSafelySkipMountPointCheck() bool { return mounter.withSafeNotMountedBehavior