Replace the manual handling of mounts with the contianerd's mount package

[cmainas]

Description

Replace all the manual handling of mounts (option parsing, mount calls, unmounting) with containerd's mount package. Furthermore, use SecureJoin to safely resolve mount targets and copy destinations inside the monitor rootfs, so symlinks in image-controlled paths cannot redirect a mount outside of it.

Since containerd does not parse propagation flags correctly (ignores them) and because the kernel ignores per-mount VFS flags when a bind mount is created, we split the mount options in three groups: the options forwarded to containerd, the propagation flags and the per-mount VFS flags. For bind mounts we reapply the VFS flags, preserving the CL_UNPRIVILEGED locked flags of the source mount when running inside a user namespace.

Also, since we switch from moby/sys/mount to containerd's mount package we also switch from a lazy unmount to a strict one, which is necessary since we want to attach the block devices to the sandbox.

At last, fix getMonitorDevices to not append an empty device entry when /dev/net/tun does not exist on the host. Leftover from #835

Related issues

• Fixes Replace all manually handled mounts with containerd's mount helper #825

How was this tested?

with e2e tests

LLM usage

Opus 4.8 to update all calls for mounts

Checklist

• I have read the contribution guide.
• The linter passes locally (make lint).
• The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl).
• If LLMs were used: I have read the llm policy.

[netlify]

✅ Deploy Preview for urunc canceled.

Name	Link
🔨 Latest commit	d47cd30
🔍 Latest deploy log	https://app.netlify.com/projects/urunc/deploys/6a5e7b1ed3502900072330a4

[netlify]

✅ Deploy Preview for urunc canceled.

Name	Link
🔨 Latest commit	0fb9e93
🔍 Latest deploy log	https://app.netlify.com/projects/urunc/deploys/6a71ca641c9b55000854f660

[ananos]

thanks @cmainas! Moving all of this to containerd's mount package is the right call. I gave it a spin on our testbed (urunc-prs-dev, kernel 6.8, containerd 2.3.3) and it looks solid overall. There are two things in the block path I would fix before we merge though, details below.

What passes

Unit tests are green both on the branch and on the branch merged with current main, it merges cleanly, and it builds. gofmt and go vet are clean too.

For the e2e side I ran all four suites as an A/B, pure builds of main and of this PR, back to back on the same machine:

                 main (9575c06)      pr-840 (c218c7c)
nerdctl    26 Passed | 0 Failed    26 Passed | 0 Failed
ctr        17 Passed | 0 Failed    17 Passed | 0 Failed
crictl     13 Passed | 0 Failed    13 Passed | 0 Failed
docker     15 Passed | 0 Failed    15 Passed | 0 Failed

So 71 specs each, fully green on both, no difference between the two. I also checked that ro / nosuid / nodev / noexec and propagation come out byte-identical in /proc/self/mountinfo on both branches, root and rootless. The rewrite does not seem to change mount semantics anywhere I could see.

One caveat on that green run, and it is the reason I am still flagging things below: neither of the two issues I found is reachable from the e2e suites. Nothing there mounts a volume with nosymfollow, and nothing holds a mountpoint busy during create. So a clean e2e is not evidence against them.

Two things got better that are worth calling out. The rootless path is genuinely improved, binding a source whose mount carries kernel-locked flags fails with operation not permitted on main and works here with ro,nosuid,nodev preserved. The /dev/net/tun fix is real too, with the device masked main dies with " is not a device node" and cannot create any container at all, while this branch is fine.

Two things I would fix before merging

1. restoreBlockVolumes fails on legal mountinfo options, and it fails silently.

We now pass the raw mountinfo field 6 string to containerd as Options. Any token that is not in containerd's flag table ends up as filesystem specific data, and ext4 rejects it. nosymfollow (kernel >= 5.10) is one of those. Calling the real function on both branches, with an actual ext4 loop device mounted -o nosymfollow:

main   : restoreBlockVolumes OK, remounted with "rw,relatime,nosymfollow"
pr-840 : restoreBlockVolumes FAILED: failed to remount /dev/loop2: invalid argument

main got away with it because splitMountOptions knew nosymfollow and it passed "" as data anyway. The reason I would treat this as blocking is that Delete() only logs the error (unikontainers.go:857), so the volume just never gets restored and nobody notices. idmapped should hit the same path, although I did not test that one.

I would run the tokens through our own mapVFSFlag before handing them to containerd. That keeps the fix local and we stay in control of what containerd never learned to parse, same rationale as the propagation flags.

2. The strict unmount can EBUSY the create path.

I get why this changed, and the PR description is clear that it is needed for attaching the block devices. Still, moby's Unmount was MNT_DETACH and could never return EBUSY, so this is a real behaviour change. With the mountpoint held busy:

pr-840 strict Unmount(path, 0)      : device or resource busy   (after 2.509s of retries)
main   lazy   Unmount(path, DETACH) : <nil>

Both call sites (getBlockVolumes and blockRootfs.preSetup in block.go) run in the host mount namespace during create. So any host process holding an fd inside the rootfs, eg. a kubelet imagefs walker or an admin shell, turns a create that always used to work into a failure, plus a 2.5s stall on the way there. Could we fall back to MNT_DETACH on EBUSY rather than propagating it? That would keep the strict semantics you need for the common case.

While we are in here: /tmp loses its sticky bit

Not critical and not caused by this PR, but it lives in code you are moving anyway, so it may be worth folding in.

os.Chmod(target, 01777) does not do what it looks like. Go encodes sticky as os.ModeSticky (1<<23), not as octal 0o1000, so the bit is silently dropped:

unix.Chmod(0o1777)          -> dtrwxrwxrwx  (sticky)
os.Chmod(01777)             -> drwxrwxrwx   (no sticky)
os.Chmod(0777|ModeSticky)   -> dtrwxrwxrwx  (sticky)

The annoying part is that the tmpfs mount already set the sticky bit via mode=1777, and this chmod then clears it. So the monitor rootfs /tmp ends up world writable without the sticky protection. os.Chmod(target, 0o777|os.ModeSticky) fixes it.

Happy to share the throwaway Go tests I used for any of the above if that helps reproduce. What do you think?

[cmainas]

Hello @ananos ,

I will pass other mounts from splitMountOptions and apply later the VFS flags that containerd ignores.

2. The strict unmount can EBUSY the create path.

I get why this changed, and the PR description is clear that it is needed for attaching the block devices. Still, moby's Unmount was MNT_DETACH and could never return EBUSY, so this is a real behaviour change. With the mountpoint held busy:

pr-840 strict Unmount(path, 0)      : device or resource busy   (after 2.509s of retries)
main   lazy   Unmount(path, DETACH) : <nil>

Both call sites (getBlockVolumes and blockRootfs.preSetup in block.go) run in the host mount namespace during create. So any host process holding an fd inside the rootfs, eg. a kubelet imagefs walker or an admin shell, turns a create that always used to work into a failure, plus a 2.5s stall on the way there. Could we fall back to MNT_DETACH on EBUSY rather than propagating it? That would keep the strict semantics you need for the common case.

I have some reservations about this suggestion. In "lazy unmount" we risk data corruption because the same volume will be attached in two different places on the same time and in "strict unmount" we risk to be slower. Also, the "fallback" would not really save us neither time nor the data corruption, we will still have to wait and get EBUSY, while then ignoring it and attach the volume to the sandbox.

We should also keep in mind if such a scenario, a process keeps a reference in that mount is realistic. In the case of rootfs this is not possible because it is created from the shim and then urunc create is called. Only urunc can hold references, which we can control. In the case of block-based volumes I honestly can not be so certain, but I think we should still avoid any data corruption and simply fail.

On the other hand, we do not really need to unmount using the containerd package, We can instead copy the unmount function of containerd (https://github.com/containerd/containerd/blob/v1.7.34/mount/mount_linux.go#L203) and reduce the timeout, so we can fail fast.

While we are in here: /tmp loses its sticky bit

Not critical and not caused by this PR, but it lives in code you are moving anyway, so it may be worth folding in.

os.Chmod(target, 01777) does not do what it looks like. Go encodes sticky as os.ModeSticky (1<<23), not as octal 0o1000, so the bit is silently dropped:

unix.Chmod(0o1777)          -> dtrwxrwxrwx  (sticky)
os.Chmod(01777)             -> drwxrwxrwx   (no sticky)
os.Chmod(0777|ModeSticky)   -> dtrwxrwxrwx  (sticky)

The annoying part is that the tmpfs mount already set the sticky bit via mode=1777, and this chmod then clears it. So the monitor rootfs /tmp ends up world writable without the sticky protection. os.Chmod(target, 0o777|os.ModeSticky) fixes it.

I will include this small change too.

[cmainas]

Hello @ananos , I have addressed the comments.