Repository: opencontainers/runc
Pull request #4740 (2025-04-22 13:36:13)

[1.2] rootfs: improve mount-related errors
This is a backport of #4734 to 1.2.x.

<hr>

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.

Also, 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.
(Though maybe @kolyshkin has an opinion on this.)

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

kolyshkin: Let's fix CI first so we get all green here: https://github.com/opencontainers/runc/pull/4742

Patch:
diff --git a/libcontainer/mount_linux.go b/libcontainer/mount_linux.go
index f2eaa937..683b5e62 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/rootfs_linux.go b/libcontainer/rootfs_linux.go
index f7cd95dd..868c199b 100644
--- a/libcontainer/rootfs_linux.go
+++ b/libcontainer/rootfs_linux.go
@@ -664,11 +664,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
@@ -689,17 +695,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)
 			}
 		}
 


Test patch:
diff --git a/libcontainer/mount_linux_test.go b/libcontainer/mount_linux_test.go
new file mode 100644
index 00000000..6bddfc2f
--- /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)
+		}
+	}
+}