Resolve container paths in the container's mount namespace

[dmcgowan]

The OCI runtime applies a container's mounts inside the container's own mount namespace, so the bundle rootfs vminitd sees backs only the paths no mount covers: copying through it writes files the container never observes and reads content it does not have. Hold a reference to each container's mount namespace and run the archive operations inside it, so the kernel resolves container paths as it does for the container's own processes, for every mount type and with ".." and absolute symlink targets anchored at the container's root. The reference also keeps a container that has exited but not yet been deleted readable, and the container's root replaces the destination directory as the boundary on extraction, now enforced by the kernel rather than by path checks.

Related to containerd/shimtest#15 (used to validate this change)

[Copilot]

🟡 Changes recommended

Critical path-resolution and lifecycle issues, plus stream leaks, remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR makes transfer operations resolve paths inside each container’s pinned mount namespace.

Changes:

• Adds and registers a per-container filesystem namespace registry.
• Pins namespaces through the container lifecycle.
• Runs archive transfers in the container filesystem context.
• Updates related tests and plugin wiring.

File summaries

File	Summary
plugins/vminit/task/plugin_linux.go	Injects the filesystem registry into the task service.
plugins/vminit/ctrfs/plugin.go	Defines the filesystem plugin identifier.
plugins/vminit/ctrfs/plugin_linux.go	Registers the filesystem registry plugin.
plugins/services/transfer/service.go	Supplies namespace-aware filesystem access to transfers.
internal/vminit/task/service.go	Critical (2 votes): Container ID reuse can release the replacement namespace and delete the replacement container.
internal/vminit/ctrfs/handle_linux.go	Critical (3 votes): setns does not change the worker’s filesystem root, allowing operations to resolve against the VM filesystem.
internal/vminit/ctrfs/ctrfs.go	Manages per-container namespace handles.
internal/vminit/ctrfs/ctrfs_linux_test.go	Tests registry lifecycle behavior.
internal/transfer/containerfs.go	Critical (1 vote): Lexically cleaning .. breaks symlink-based container path resolution. Moderate (1 vote): Acquired streams can leak when Do fails. Moderate (1 vote): Streams can also leak when no registry entry exists.
internal/transfer/containerfs_transfer_test.go	Tests unavailable-container transfer handling.
internal/transfer/containerfs_test.go	Updates archive behavior tests for namespace-root semantics.
cmd/vminitd/main.go	Loads the filesystem plugin.

Review details

Suppressed comments (2)

internal/transfer/containerfs.go:80

• The stream is already created by the client and acquired by convertAny before this closure runs; Do returning ErrNotFound therefore does not mean no stream was created. On that path neither d.Writer nor s.Reader is called, so the acquired stream is never closed, leaving the vsock bridge/client goroutine blocked and leaking a stream for every unreachable container. Add cleanup for the acquired stream on Do failure (or otherwise close it before returning).

		// Set the stream up inside the closure so that a container whose
		// filesystem cannot be reached is reported without a stream having
		// been created for it. Stream setup performs no path resolution, so
		// running it here does not depend on which filesystem is in view.

internal/transfer/containerfs.go:94

• If the registry has no entry, this callback is never entered, so the ReadStream acquired during convertAny is neither consumed nor closed. Its client sender and the VM-side vsock bridge can remain blocked indefinitely, leaking one stream per failed copy-to request; close the acquired stream when Do fails.

		return t.ctrFS.Do(d.ContainerID, func() error {
			return readPath(s.Reader(ctx), containerRoot, d.Path, s.MediaType, d.PreserveOwnership)

• Files reviewed: 12/12 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new mount-namespace execution path has correctness/safety issues (notably setns/root-cwd requirements and a panic-induced deadlock risk) that can break transfers or hang callers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/transfer/containerfs.go:70

• containerFSTransferrer.Transfer dereferences t.ctrFS without checking for nil; if NewContainerFSTransferrer is called with a nil implementation this will panic. Returning an InvalidArgument error makes failures easier to diagnose and avoids crashing the transfer service.

func (t *containerFSTransferrer) Transfer(ctx context.Context, src, dst any, opts ...ctransfer.Opt) error {
	switch s := src.(type) {

• Files reviewed: 13/13 changed files
• Comments generated: 3
• Review effort level: Lite

[austinvazquez]

Alternative to #287

[Copilot]

🟡 Changes recommended

Critical container-boundary and namespace-lifecycle issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

internal/transfer/containerfs.go:119

• rootRel still collapses .. lexically before the kernel resolves the path. That changes container semantics through symlinks: if /link points to var/sub, a process opening /link/../file reaches /var/file, whereas this normalization turns it into /file. Preserve the components for kernel resolution (with a separate root-confined resolution mechanism) so .. behaves as it does inside the container.

// rootRel converts a path expressed in the container's view, which may be
// absolute or contain parent-directory components, into a path relative to the
// filesystem root. Cleaning is lexical: "../" sequences collapse before the
// path is resolved rather than as the container would resolve them, so the
// result never names anything above the root. An empty result becomes ".",

internal/transfer/pseudofs_linux.go:36

• The recursive walk skips only the filesystem types in this table. Standard container mounts such as /dev/pts (devpts) and /dev/mqueue (mqueue) are kernel-generated but are not listed, so exporting / still descends into them; queue/control entries can fail to read or produce unusable archive contents. Include the remaining pseudo-filesystem types or skip them from mount metadata rather than assuming this list is exhaustive.

var pseudoFSTypes = map[uint64]string{
	unix.PROC_SUPER_MAGIC:    "proc",
	unix.SYSFS_MAGIC:         "sysfs",
	unix.CGROUP_SUPER_MAGIC:  "cgroup",
	unix.CGROUP2_SUPER_MAGIC: "cgroup2",

• Files reviewed: 17/17 changed files
• Comments generated: 7
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate review findings must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

internal/transfer/containerfs.go:120

• The new namespace-backed path is still normalized lexically before the kernel resolves it. For example, if /alias in the container points to /a/b, a request for /alias/../file is cleaned to /file, while a container process resolves it to /a/file. That contradicts the stated container-view semantics for .. components; pass the uncleaned path to the namespace resolver while relying on the container root to bound traversal, or explicitly narrow the API contract.

// absolute or contain parent-directory components, into a path relative to the
// filesystem root. Cleaning is lexical: "../" sequences collapse before the
// path is resolved rather than as the container would resolve them, so the
// result never names anything above the root. An empty result becomes ".",
// the root itself.

internal/transfer/containerfs.go:304

• The destination is created before its filesystem is checked. A destination below a pseudo-filesystem (for example a new directory under debugfs) can therefore be created or otherwise modified before pseudoFS rejects the transfer, and failures on filesystems that disallow mkdir are reported as ordinary mkdir errors. Check pseudoFSHolder(dst) before calling MkdirAll.

	dst := rootJoin(root, rootRel(dstPath))
	if err := os.MkdirAll(dst, 0755); err != nil {
		return fmt.Errorf("failed to create destination: %w", err)
	}
	// Extracting here would write into kernel interfaces rather than

internal/transfer/containerfs.go:350

• This check only examines the parent directory, so an archive entry naming a pseudo-filesystem mount point itself (for example proc/) is not skipped. With ownership preservation, the later Lchown can operate on /proc; other entry types can fail or try to replace the mount. Check the entry path as well as its parent before extracting, while still allowing a symlink entry to be archived literally.

		if pseudoHolder(filepath.Dir(filepath.Join(dst, entryName))) != "" {
			continue
		}

internal/transfer/containerfs.go:53

• This contract claims the operation is confined to the container filesystem, but ordinary absolute path lookup still follows procfs magic links. For a container that shares vminitd's PID namespace, /proc/1/root/... can resolve to vminitd's root, allowing copy-in/out to read or write outside the container root despite the mount namespace switch. Use descriptor-relative resolution with RESOLVE_IN_ROOT|RESOLVE_NO_MAGICLINKS, or otherwise prevent magic-link escapes.

// Implementations confine the operation to the container's filesystem, so
// paths cannot reach anything outside it.

internal/vminit/ctrfs/handle_linux.go:141

• Each Do creates a goroutine, locks its OS thread, and deliberately returns without UnlockOSThread; the Go runtime terminates that locked thread when the goroutine exits. Every transfer therefore pays an OS-thread create/teardown cost, and concurrent transfers consume one dedicated thread each, which can become a scalability bottleneck. Reuse a namespace-bound worker or restore the original namespace/fs state before unlocking a reusable thread.

		runtime.LockOSThread()
		// Never unlocked. Joining is irreversible for this thread, and the
		// runtime terminates a locked thread once its goroutine returns, so
		// returning is what retires the thread rather than handing it back
		// carrying the container's filesystem view.

• Files reviewed: 17/17 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved path-confinement, pseudo-filesystem, and stream-cleanup issues remain.

Review details

Suppressed comments (8)

internal/transfer/containerfs.go:304

• MkdirAll runs before pseudoFS is checked, so a non-existent destination below a writable pseudo-filesystem can be created as a kernel object before the transfer is rejected (for example, /sys/fs/cgroup/new creates a cgroup directory and then returns InvalidArgument). Check pseudoFSHolder(dst) before creating dst (and retain the post-create check for races) so refusing pseudo-filesystems has no side effects.

	if err := os.MkdirAll(dst, 0755); err != nil {
		return fmt.Errorf("failed to create destination: %w", err)

internal/transfer/containerfs.go:143

• filepath.Join cleans rel, so a path such as /link/../file is normalized before the kernel sees it. If /link is a symlink to /mnt/data, a container resolves that path as /mnt/file, while this code accesses /file; this violates the container-path resolution contract for .. components following symlinks. Preserve those components for kernel resolution and handle archive-name containment separately.

func rootJoin(root, rel string) string {
	if rel == "." {
		return root
	}
	return filepath.Join(root, rel)

internal/transfer/containerfs.go:352

• This checks only the entry's parent, so an archive entry naming an existing mountpoint itself (for example proc when extracting at /) sees / as ordinary and reaches extractTarEntry. A directory entry can then be chowned, while a regular or symlink entry tries to remove /proc and fails with EBUSY instead of skipping the pseudo-filesystem. Check the entry path itself (with the nearest-ancestor fallback) before extracting.

		if pseudoHolder(filepath.Dir(filepath.Join(dst, entryName))) != "" {
			continue
		}

internal/transfer/pseudofs_linux.go:71

• On 32-bit Linux, unix.Statfs_t.Type is an int32; converting it directly to uint64 sign-extends filesystem magic values with the high bit set. Consequently securityfs, selinuxfs, efivarfs, and hugetlbfs are not recognized and can be archived or extracted as ordinary filesystems. Normalize the statfs value to its 32-bit representation before the map lookup.

	return pseudoFSTypes[uint64(st.Type)]

internal/transfer/types.go:136

• Closing the raw stream here does not reliably stop a client-side ReadStream producer. The transfer streaming SendStream loop treats a closed window channel as repeated zero updates and continues reading/sending instead of observing the close, so a failed copy-to can leave the client blocked on a large or unbounded source after this method returns. The close path needs a cancellation/terminal signal that the sender loop consumes (or the sender loop must handle the closed window channel).

func (s *ReadStream) Close() error {
	if s.stream == nil {
		return nil
	}
	return s.stream.Close()

internal/transfer/types.go:132

• These stream cleanup methods are only called inside containerFSTransferrer after both convertAny calls and transferrer selection have succeeded. If destination unmarshalling fails after a source stream was acquired, or if no transferrer handles the pair, service.Transfer returns without closing either stream; the vminit stream manager retains it and the peer can wait or remain blocked indefinitely. Cleanup needs to be deferred at the service boundary once a stream-bearing operand has been decoded, not limited to ctrFS.Do errors.

// Close releases the stream without consuming it, so that a client
// sending on it is not left waiting when the transfer cannot be carried
// out.
//
// The stream is established while the request is being unmarshalled,
// before any transferrer sees it, so a transfer that fails before
// reading has to release it explicitly. Close is safe to call once the
// stream has been read: consuming it closes the stream too, and the
// transport tolerates the second close.
func (s *ReadStream) Close() error {

internal/vminit/ctrfs/ctrfs.go:42

• This caveat is an exploitable boundary escape, not just a difference from container credentials. With a shared PID namespace, a client-supplied path such as /proc/1/root/etc/shadow resolves through the host PID 1's magic link; the same path in an imported tar can write through to the VM. setns/the fs root does not constrain proc magic links or inherited /proc/self/fd handles, so this violates the ContainerFS confinement promise. Use descriptor-relative resolution with RESOLVE_IN_ROOT/RESOLVE_NO_MAGICLINKS, or reject these paths, before exposing this API.

// cannot ascend above the container's root, but procfs magic links such as
// /proc/<pid>/root are not resolution, and where those lead depends on whether
// the container was given its own PID namespace.

internal/vminit/ctrfs/handle_linux.go:153

• setns(CLONE_NEWNS) sets the worker's fs root to the mount namespace's root mount, not to the container process's fs_struct root. This breaks the supported NoPivotRoot option: crun uses chroot without pivoting the namespace root, so the namespace root remains the VM root and containerRoot (/) makes copy operations target the VM filesystem. Pin the init's /proc/<pid>/root (or reject NoPivotRoot) and establish that directory as the worker root before calling fn.

		if err := unix.Setns(fd, unix.CLONE_NEWNS); err != nil {
			errCh <- fmt.Errorf("failed to join mount namespace: %w", err)
			return
		}

• Files reviewed: 17/17 changed files
• Comments generated: 0 new
• Review effort level: Lite