From 4f7de1e0d34ba8620c9703fdef27c57525156595 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 18 Apr 2025 14:10:03 +1000 Subject: [PATCH 1/2] mount: add string representation of mount flags When reading mount errors, it is quite hard to make sense of mount flags in their hex form. As this is the error path, the minor performance impact of constructing a string is probably not worth hyper-optimising. Signed-off-by: Aleksa Sarai (cherry picked from commit 30302a2850c2b64931bc725902361e1c53a7eacf) Signed-off-by: Kir Kolyshkin --- libcontainer/mount_linux.go | 61 +++++++++++++++++++++++++++++++- libcontainer/mount_linux_test.go | 54 ++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 libcontainer/mount_linux_test.go diff --git a/libcontainer/mount_linux.go b/libcontainer/mount_linux.go index f2eaa937e..683b5e624 100644 --- a/libcontainer/mount_linux.go +++ b/libcontainer/mount_linux.go @@ -6,6 +6,7 @@ import ( "io/fs" "os" "strconv" + "strings" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -45,6 +46,64 @@ type mountError struct { err error } +// int32plus is a collection of int types with >=32 bits. +type int32plus interface { + int | uint | int32 | uint32 | int64 | uint64 | uintptr +} + +// stringifyMountFlags converts mount(2) flags to a string that you can use in +// error messages. +func stringifyMountFlags[Int int32plus](flags Int) string { + flagNames := []struct { + name string + bits Int + }{ + {"MS_RDONLY", unix.MS_RDONLY}, + {"MS_NOSUID", unix.MS_NOSUID}, + {"MS_NODEV", unix.MS_NODEV}, + {"MS_NOEXEC", unix.MS_NOEXEC}, + {"MS_SYNCHRONOUS", unix.MS_SYNCHRONOUS}, + {"MS_REMOUNT", unix.MS_REMOUNT}, + {"MS_MANDLOCK", unix.MS_MANDLOCK}, + {"MS_DIRSYNC", unix.MS_DIRSYNC}, + {"MS_NOSYMFOLLOW", unix.MS_NOSYMFOLLOW}, + // No (1 << 9) flag. + {"MS_NOATIME", unix.MS_NOATIME}, + {"MS_NODIRATIME", unix.MS_NODIRATIME}, + {"MS_BIND", unix.MS_BIND}, + {"MS_MOVE", unix.MS_MOVE}, + {"MS_REC", unix.MS_REC}, + // MS_VERBOSE was deprecated and swapped to MS_SILENT. + {"MS_SILENT", unix.MS_SILENT}, + {"MS_POSIXACL", unix.MS_POSIXACL}, + {"MS_UNBINDABLE", unix.MS_UNBINDABLE}, + {"MS_PRIVATE", unix.MS_PRIVATE}, + {"MS_SLAVE", unix.MS_SLAVE}, + {"MS_SHARED", unix.MS_SHARED}, + {"MS_RELATIME", unix.MS_RELATIME}, + // MS_KERNMOUNT (1 << 22) is internal to the kernel. + {"MS_I_VERSION", unix.MS_I_VERSION}, + {"MS_STRICTATIME", unix.MS_STRICTATIME}, + {"MS_LAZYTIME", unix.MS_LAZYTIME}, + } + var ( + flagSet []string + seenBits Int + ) + for _, flag := range flagNames { + if flags&flag.bits == flag.bits { + seenBits |= flag.bits + flagSet = append(flagSet, flag.name) + } + } + // If there were any remaining flags specified we don't know the name of, + // just add them in an 0x... format. + if remaining := flags &^ seenBits; remaining != 0 { + flagSet = append(flagSet, "0x"+strconv.FormatUint(uint64(remaining), 16)) + } + return strings.Join(flagSet, "|") +} + // Error provides a string error representation. func (e *mountError) Error() string { out := e.op + " " @@ -62,7 +121,7 @@ func (e *mountError) Error() string { } if e.flags != uintptr(0) { - out += ", flags=0x" + strconv.FormatUint(uint64(e.flags), 16) + out += ", flags=" + stringifyMountFlags(e.flags) } if e.data != "" { out += ", data=" + e.data diff --git a/libcontainer/mount_linux_test.go b/libcontainer/mount_linux_test.go new file mode 100644 index 000000000..6bddfc2f0 --- /dev/null +++ b/libcontainer/mount_linux_test.go @@ -0,0 +1,54 @@ +package libcontainer + +import ( + "testing" + + "golang.org/x/sys/unix" +) + +func TestStringifyMountFlags(t *testing.T) { + for _, test := range []struct { + name string + flags uintptr + expected string + }{ + {"Empty", 0, ""}, + // Single valid flags. + {"Single-MS_RDONLY", unix.MS_RDONLY, "MS_RDONLY"}, + {"Single-MS_NOSUID", unix.MS_NOSUID, "MS_NOSUID"}, + {"Single-MS_NODEV", unix.MS_NODEV, "MS_NODEV"}, + {"Single-MS_NOEXEC", unix.MS_NOEXEC, "MS_NOEXEC"}, + {"Single-MS_SYNCHRONOUS", unix.MS_SYNCHRONOUS, "MS_SYNCHRONOUS"}, + {"Single-MS_REMOUNT", unix.MS_REMOUNT, "MS_REMOUNT"}, + {"Single-MS_MANDLOCK", unix.MS_MANDLOCK, "MS_MANDLOCK"}, + {"Single-MS_DIRSYNC", unix.MS_DIRSYNC, "MS_DIRSYNC"}, + {"Single-MS_NOSYMFOLLOW", unix.MS_NOSYMFOLLOW, "MS_NOSYMFOLLOW"}, + {"Single-MS_NOATIME", unix.MS_NOATIME, "MS_NOATIME"}, + {"Single-MS_NODIRATIME", unix.MS_NODIRATIME, "MS_NODIRATIME"}, + {"Single-MS_BIND", unix.MS_BIND, "MS_BIND"}, + {"Single-MS_MOVE", unix.MS_MOVE, "MS_MOVE"}, + {"Single-MS_REC", unix.MS_REC, "MS_REC"}, + {"Single-MS_SILENT", unix.MS_SILENT, "MS_SILENT"}, + {"Single-MS_POSIXACL", unix.MS_POSIXACL, "MS_POSIXACL"}, + {"Single-MS_UNBINDABLE", unix.MS_UNBINDABLE, "MS_UNBINDABLE"}, + {"Single-MS_PRIVATE", unix.MS_PRIVATE, "MS_PRIVATE"}, + {"Single-MS_SLAVE", unix.MS_SLAVE, "MS_SLAVE"}, + {"Single-MS_SHARED", unix.MS_SHARED, "MS_SHARED"}, + {"Single-MS_RELATIME", unix.MS_RELATIME, "MS_RELATIME"}, + {"Single-MS_KERNMOUNT", unix.MS_KERNMOUNT, "0x400000"}, + {"Single-MS_I_VERSION", unix.MS_I_VERSION, "MS_I_VERSION"}, + {"Single-MS_STRICTATIME", unix.MS_STRICTATIME, "MS_STRICTATIME"}, + {"Single-MS_LAZYTIME", unix.MS_LAZYTIME, "MS_LAZYTIME"}, + // Invalid flag value. + {"Unknown-512", 1 << 9, "0x200"}, + // Multiple flag values at the same time. + {"Multiple-Valid1", unix.MS_RDONLY | unix.MS_REC | unix.MS_BIND, "MS_RDONLY|MS_BIND|MS_REC"}, + {"Multiple-Valid2", unix.MS_NOSUID | unix.MS_NODEV | unix.MS_NOEXEC | unix.MS_REC | unix.MS_NODIRATIME | unix.MS_I_VERSION, "MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_NODIRATIME|MS_REC|MS_I_VERSION"}, + {"Multiple-Mixed", unix.MS_REC | unix.MS_BIND | (1 << 9) | (1 << 31), "MS_BIND|MS_REC|0x80000200"}, + } { + got := stringifyMountFlags(test.flags) + if got != test.expected { + t.Errorf("%s: stringifyMountFlags(0x%x) = %q, expected %q", test.name, test.flags, got, test.expected) + } + } +} From 708fe7615fbb109e10ec97eeb135ed11b2f760eb Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 18 Apr 2025 14:11:28 +1000 Subject: [PATCH 2/2] rootfs: improve error messages for bind-mount vfs flag setting While debugging an issue involving failing mounts, I discovered that just returning the plain mount error message when we are in the fallback code for handling locked mounts leads to unnecessary confusion. It also doesn't help that podman currently forcefully sets "rw" on mounts, which means that rootless containers are likely to hit the locked mounts issue fairly often. So we should improve our error messages to explain why the mount is failing in the locked flags case. Fixes: 7c71a2270547 ("rootfs: remove --no-mount-fallback and finally fix MS_REMOUNT") Signed-off-by: Aleksa Sarai (cherry picked from commit 58c3ab77b0ab39f25d38578a96e0ca36ea60be13) Signed-off-by: Kir Kolyshkin --- libcontainer/rootfs_linux.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index e46109a2f..dc0fb7193 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -665,11 +665,17 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { return err } srcFlags := statfsToMountFlags(*st) + + logrus.Debugf( + "working around failure to set vfs flags on bind-mount %s: srcFlags=%s flagsSet=%s flagsClr=%s: %v", + m.Destination, stringifyMountFlags(srcFlags), + stringifyMountFlags(m.Flags), stringifyMountFlags(m.ClearedFlags), mountErr) + // If the user explicitly request one of the locked flags *not* // be set, we need to return an error to avoid producing mounts // that don't match the user's request. - if srcFlags&m.ClearedFlags&mntLockFlags != 0 { - return mountErr + if cannotClearFlags := srcFlags & m.ClearedFlags & mntLockFlags; cannotClearFlags != 0 { + return fmt.Errorf("cannot clear locked flags %s: %w", stringifyMountFlags(cannotClearFlags), mountErr) } // If an MS_*ATIME flag was requested, it must match the @@ -690,17 +696,19 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { // MS_STRICTATIME mounts even if the user requested MS_RELATIME // or MS_NOATIME. if m.Flags&mntAtimeFlags != 0 && m.Flags&mntAtimeFlags != srcFlags&mntAtimeFlags { - return mountErr + return fmt.Errorf("cannot change locked atime flags %s: %w", stringifyMountFlags(srcFlags&mntAtimeFlags), mountErr) } // Retry the mount with the existing lockable mount flags // applied. flags |= srcFlags & mntLockFlags mountErr = mountViaFds("", nil, m.Destination, dstFd, "", uintptr(flags), "") - logrus.Debugf("remount retry: srcFlags=0x%x flagsSet=0x%x flagsClr=0x%x: %v", srcFlags, m.Flags, m.ClearedFlags, mountErr) + if mountErr != nil { + mountErr = fmt.Errorf("remount with locked flags %s re-applied: %w", stringifyMountFlags(srcFlags&mntLockFlags), mountErr) + } return mountErr }); err != nil { - return err + return fmt.Errorf("failed to set user-requested vfs flags on bind-mount: %w", err) } }