Fix block and ref rootfs

[cmainas]

Description

This PR takes care of various bugs we had with the handling of the rootfs, but all related to the related issue below. The main problem was that when the reexec process was unmounting the block-based snapshot which was mounted as the container;s rootfs in the host, the unmounting was not propagating to the other mount namespaces (especially the initial one where the mounting of the snapshot was taking place).

The thing is that usually the container;s rootfs in the host is mounted with MS_SHARED propagation flag. Therefore all mount / unmount events taking place in subsequent mount namespaces (or peer mount groups) should also propagate. In simple words the unmount in reexec should propagate, except if something was cutting of the propagation.

Well, indeed something was cutting off the propagation (See https://github.com/urunc-dev/urunc/blob/main/pkg/unikontainers/unikontainers.go#L341). As part of preparing the rootfs for the execution environment of the sandbox monitor, we had to set the propagation flags according to the spec we received for the new mount namespace and make sure that the parent of the monitor's rootfs in the host will be MS_PRIVATE. This is necessary for pivot to work.

All the above are necessary steps, but they cut off the mount propagation of mounts and when we perform the unmount later in https://github.com/urunc-dev/urunc/blob/main/pkg/unikontainers/unikontainers.go#L362 the unmount will never propagate. Subsequently the solution is simple: make sure the rootfs of the monitor process in the host does have MS_SHARED as propagation flag and unmount before cutting off the mount propagation.

Furthermore, to avoid any further surprises with block based mounts and rootfs, a simple check is added in getMountInfo to always exclude mount sources which appear more than once in /proc/self/mountinfo.

Due to the high cognitive complexity of Exec a quick fix would add extra complexity as shown in the 3rd commit. Since we are already planning to perform a major refactor for Exec and unikontainers, I brought parts of this refactor in this PR, in order to help with the further refactoring later.

At last, there was a bug in checking for a specific namespace since findNS was also checking for the path of the namespace in /proc and hence failing for new namespaces that the runtime needs to create. As a result, when we had to create a mount namespace we were not pivoting but chroot.

Related issues

• Fixes Block based rootfs is not unmounted from host before attaching to sandbox #562

How was this tested?

With the end-to-end tests and to make sure the unmounting propagates, after running a urunc container which can accept a block-based device as the sandbox's rootfs the mount command should not show this block image mounted.

LLM usage

N/A

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	f71e10a
🔍 Latest deploy log	https://app.netlify.com/projects/urunc/deploys/69f879b3aa0f900008ed58ab

[ananos]

thanks @cmainas.

The MS_SHARED fix in InitialSetup is correct — verified that the block rootfs unmount now propagates and #562 is resolved.

However, the findNS change is doing the opposite of what the description says. Quick recap of the description:

"when we had to create a mount namespace we were not pivoting but chroot".

I built both branches (main & PR) with a probe at the call site and ran rumprun-hvt via ctr run:

  main:    findNS(mount)=("", err="the namespace does not exist")             
           withPivot=true  -> PIVOT_ROOT                            
  this PR: findNS(mount)=("", err=<nil>)                                      
           withPivot=false -> CHROOT

Same OCI spec in both runs:

namespaces=[{Type:pid} {Type:ipc} {Type:uts} {Type:mount} {Type:network}]

The standard containerd shape with empty Path on the mount ns.

main was already pivoting in that case; this PR changes it to chroot.

Cause: findNS no longer errors on empty Path, but the call site in Exec is unmodified:

    _, err = findNS(u.Spec.Linux.Namespaces, specs.MountNamespace)  
    withPivot := err != nil   // was a side-effect of the old findNS contract

The comment above this block was already inverted from the code on main, so
the original intent isn't fully clear. I think you want:

    nsPath, err := findNS(u.Spec.Linux.Namespaces, specs.MountNamespace)
    // own the ns (in spec, no path) -> pivot; joining an existing ns -> chroot                                                                               
    withPivot := err == nil && nsPath == ""                                   

Would it make sense to split this PR? I'd merge the MS_SHARED + getMountInfo + refactor
parts as-is, and address the findNS / pivot-vs-chroot question in a separate
PR with a test, since it's an isolation-level change rather than a bug fix. Maybe I'm missing something in the process, so feel free to ignore the findNS analysis. Regardless, let's try to expedite the merge!

[cmainas]

Hello @ananos ,

thank you for the review. That was a good catch.

However, the findNS change is doing the opposite of what the description says. Quick recap of the description:

"when we had to create a mount namespace we were not pivoting but chroot".

I built both branches (main & PR) with a probe at the call site and ran rumprun-hvt via ctr run:

  main:    findNS(mount)=("", err="the namespace does not exist")             
           withPivot=true  -> PIVOT_ROOT                            
  this PR: findNS(mount)=("", err=<nil>)                                      
           withPivot=false -> CHROOT

Same OCI spec in both runs:

namespaces=[{Type:pid} {Type:ipc} {Type:uts} {Type:mount} {Type:network}]

The standard containerd shape with empty Path on the mount ns.

main was already pivoting in that case; this PR changes it to chroot.

Cause: findNS no longer errors on empty Path, but the call site in Exec is unmodified:

You are correct. My description was wrong. The problem is not when we create a new mount namespace, but when we join a new mount namespace. In that case, findNS returns a nil error and due tto ``withPivot := err != nil , withPivot` becomes false and hence we chroot, instead of pivot

I think you want:

    nsPath, err := findNS(u.Spec.Linux.Namespaces, specs.MountNamespace)
    // own the ns (in spec, no path) -> pivot; joining an existing ns -> chroot                                                                               
    withPivot := err == nil && nsPath == ""                                   

Yes, that is correct, I did not rebase correctly and over time I lost this change.

Would it make sense to split this PR? I'd merge the MS_SHARED + getMountInfo + refactor parts as-is, and address the findNS / pivot-vs-chroot question in a separate PR with a test, since it's an isolation-level change rather than a bug fix. Maybe I'm missing something in the process, so feel free to ignore the findNS analysis. Regardless, let's try to expedite the merge!

Sure, I can drop the findNS change and make a new PR for it with an issue.

[ananos]

thanks @cmainas!