libct: switch to new mount api by lifubang · Pull Request #5378 · opencontainers/runc

lifubang · 2026-07-21T13:15:53Z

Since the new mount API, such as open_tree, fsopen, fsmount, and move_mount, has matured over the years, it's time to embrace it fully.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

libcontainer/rootfs_linux.go:1366

sharedMaskSrc is always created as mountSourcePlain from sharedMaskFile, but when maskDir() returns a non-nil fd it is a detached mount fd (open_tree/fsmount). Detached mount fds cannot be used as a bind-mount source via mount(2) (/proc/.../fd/N), so later mountViaFds(..., sharedMaskSrc, ..., MS_BIND, ...) can fail or bind the wrong thing. To keep shared masking working, always reopen the mounted directory (reopenAfterMount) to get an O_PATH handle suitable for mountSourcePlain, and close any detached mount fd returned by maskDir().

					sharedMaskSrc = &mountSource{Type: mountSourcePlain, file: sharedMaskFile}

lifubang · 2026-07-21T16:41:10Z

+	fd, err := unix.OpenTree(-int(unix.EBADF), "/", unix.OPEN_TREE_CLOEXEC)
+	if err != nil {
+		return false
+	}
+	_ = unix.Close(fd)


This function is copied from github.com/cyphar/filepath-securejoin:
https://github.com/cyphar/filepath-securejoin/blob/main/pathrs-lite/internal/linux/mount_linux.go#L23-L47

kolyshkin

For the first commit, I'd like to add the following into the commit message (assuming it's true).

No functional change, just moving the code around.

kolyshkin · 2026-07-21T18:11:15Z

-func checkProcMount(rootfs, dest string, m mountEntry) error {
+func checkProcMount(rootfs, dest string, m *mountEntry) error {


What is the reason for this change? Just unification?

Yes, it's not necessary to make this change, so reverted it now.

lifubang · 2026-07-26T05:15:57Z

Add a commit libct: always ignore mount label for bind mounts to align the legacy mount(2) behavior with the new mount API for bind mounts.

lifubang · 2026-07-26T05:23:58Z

+// file descriptor fsfd using fsconfig(2). The options are expected to be a
+// comma-separated list of key=value pairs or flags.
+func fsconfigApplyOptions(fsfd int, opts string) error {
+	fields := strings.SplitSeq(opts, ",")


On another note, since unix.FsconfigSet(String/Flag) requires individual calls, we need to re-parse back the value of m.Data. It might be better to pass the original array directly instead of joining it here:
https://github.com/opencontainers/runc/blob/main/libcontainer/specconv/spec_linux.go#L1181-L1185

However, configs.Mount is widely used by other projects, so changing m.Data from string to []string might break compatibility.

So, what if a value contains a comma?

E.g. context="system_u:object_r:container_file_t:s0:c344,c805" or lowerdir=dir1\,dir2

lifubang · 2026-07-30T12:29:47Z

@opencontainers/runc-maintainers PTAL, Any chance we can get this into 1.6.0?

mount_entry has become complex and requires further refactoring. Extract it into a separate file to improve modularity and maintainability. No functional change, just moving the code around. Signed-off-by: lifubang <lifubang@acmcoder.com>

This change avoids duplicate fd closing and lays the groundwork for migrating to the new mount API by centralizing resource cleanup logic. Signed-off-by: lifubang <lifubang@acmcoder.com>

Adds a regression test to guarantee mount labels are respected for all supported mount types. Signed-off-by: lifubang <lifubang@acmcoder.com>

As documented in mount(2): if MS_BIND is included in mountflags, the filesystem type and mount data arguments are ignored. Therefore, keep mountLabel empty for bind mounts. Note: mount data is also blocked in the config validator: https://github.com/opencontainers/runc/blob/main/libcontainer/configs/validate/validator.go#L373-L375 Signed-off-by: lifubang <lifubang@acmcoder.com>

Signed-off-by: lifubang <lifubang@acmcoder.com>

kolyshkin

A general note: maybe we need a way to

enforce the fsmount path (i.e. exclude fallback);
enforce the fallback path (i.e. exclude fsmount);

in CI so we can definitely check both paths. Perhaps an environment variable or some such. Otherwise we won't be able to consistently test both paths.

kolyshkin · 2026-08-27T18:11:00Z

 			}
 			if bindFailed || sharedMaskSrc == nil {
-				err = maskDir(path, mountLabel)
+				fd, err := maskDir(dstFh, path, mountLabel)


Looks like a real bug. This new err declaration shadows the one which the maskPaths returns.

In other words, non-nil err here is never used / returned.

Can be fixed by e.g.:

var fd *os.File fd, err = maskDir(...)

or perhaps some refactoring

kolyshkin · 2026-08-27T18:20:01Z

-		return mountViaFds(m.Source, nil, m.Destination, dstFd, "cgroup2", uintptr(m.Flags), m.Data)
-	})
+	// We can't set mount label on cgroup2 mounts, so we just pass an empty string here.
+	err := m.mountPropagate(c.root, "")


Seems like a real bug here. The old code used to pass "cgroup2" to mountViaFds explicitly.

The new code calls mountViaFds from m.mountPropagate and uses m.Device which is "cgroup" not "cgroup2", so the fall back won't work.

Most probably we can't see this in CI due to recent distros/kernels.

Most probably we can't see this in CI due to recent distros/kernels.

Which leads to a question: are we at all able to actually check the fallbacks?

kolyshkin · 2026-08-27T19:53:54Z

+// file descriptor fsfd using fsconfig(2). The options are expected to be a
+// comma-separated list of key=value pairs or flags.
+func fsconfigApplyOptions(fsfd int, opts string) error {
+	fields := strings.SplitSeq(opts, ",")


So, what if a value contains a comma?

E.g. context="system_u:object_r:container_file_t:s0:c344,c805" or lowerdir=dir1\,dir2

kolyshkin · 2026-08-27T20:07:19Z

+				pflags |= unix.AT_RECURSIVE
 			}
+			// Only include the propagation type, not MS_REC
+			propagation |= uint64(pflag & ^unix.MS_REC)


When reading this code it seems that propagation is a mask of various values (since we have a for loop here). In fact, it's just a single value (one of MS_PRIVATE, MS_SLAVE, MS_SHARED, MS_UNBINDABLE) but for some reason we have m.PropagationFlags as a slice.

NOTE this is not the issue with the new code, but rather the issue with the existing code

So for example if m.PropagationFlags is shared,rslave the old code will apply shared then rslave and the new one will call mount_setattr with ropagation set to MS_SHARED|MS_SLAVE which will result in a debug-warning and a fallback. Maybe we should only take the last value from the slice? Or, even better, deny PropagationFlags that have more than one element (as it does not make sense).

kolyshkin · 2026-08-27T20:39:02Z

 	if flags&unix.MS_LAZYTIME != 0 {
-		logrus.Warnf("MS_LAZYTIME mount flag specified, but kernel does not support MOUNT_ATTR_LAZYTIME")
+		logrus.Warnf("MS_LAZYTIME mount flag specified, but kernel does not support MOUNT_ATTR_LAZYTIME; ignoring")


So here we're ignoring MS_LAZYTIME but in mountToRootfs (case "bind") we fall back to the old implementation if MS_LAZYTIME is set. I think we should be consistent and stick to either ignoring or falling back.

There are also MS_SYNCHRONOUS, MS_DIRSYNC, MS_MANDLOCK, and MS_SILENT which we ignore as there's no mapping to MOUNT_ATTR_*. From the list, MS_MANDLOCK seems to be deprecated but others are not. What should we do about those?

kolyshkin · 2026-08-27T20:46:04Z

+
+// createDetachedBindMount creates a detached bind mount using open_tree(2).
+func createDetachedBindMount(m *configs.Mount) (_ *mountSource, retErr error) {
+	openTreeFlags := uint(unix.OPEN_TREE_CLONE | unix.OPEN_TREE_CLOEXEC)


So we are now using OPEN_TREE_CLONE for every maskDir call. OTOH the mountFd docstring says

// Ideally, we would use OPEN_TREE_CLONE for everything, because we can // be sure that the file descriptor cannot be used to escape outside of // the mount root. Unfortunately, OPEN_TREE_CLONE is far more expensive // than open(2) because it requires doing mounts inside a new anonymous // mount namespace. So we use open(2) for standard bind-mounts, and // OPEN_TREE_CLONE when we need to set mount attributes here.

Does this mean we are slower now than we used to be?

kolyshkin · 2026-08-27T20:47:34Z

+// hasNewMountAPI reports whether the running kernel supports the new mount API
+// (open_tree(2), move_mount(2), fsopen(2), fsmount(2)), which was introduced
+// in Linux 5.2.
+var hasNewMountAPI = sync.OnceValue(func() bool {


Perhaps makes sense to mention this is a copy paste from cyphar/filepath-securejoin/pathrs-lite/internal/linux/mount_linux.go

kolyshkin · 2026-08-28T04:45:36Z

@lifubang added some comments

lifubang added the kind/refactor refactoring label Jul 21, 2026

lifubang force-pushed the feat-new-mount-api branch from ef4fecf to 3630a53 Compare July 21, 2026 13:16

lifubang requested a review from Copilot July 21, 2026 13:20

Copilot started reviewing on behalf of lifubang July 21, 2026 13:21 View session