diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e2d8e548f7..7fea3355201 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed ### +- Running a container which does not have its own mount namespace no longer + changes the propagation of host's `/` and of the parent mount of the + container rootfs (they were made private or slave). (#5537) +- For a container which does not have its own mount namespace, the container + rootfs and all its mounts are now unmounted on container deletion. (#5538) + ## [1.6.0-rc.1] - 2026-10-06 > Lo bueno, si breve, dos veces bueno. diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 5d97e9141d3..908593988dd 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -43,6 +43,9 @@ type Container struct { state containerState created time.Time fifo *os.File + // Mount ID of container's rootfs mount created by runc in the host + // mount namespace (if the container does not have its own one). + rootfsMountID uint64 } // State represents a running container's state @@ -77,6 +80,11 @@ type State struct { // Empty if the container does not have aindividual dedicated monitoring // group. IntelRdtMonPath string `json:"intel_rdt_mon_path,omitempty"` + + // Mount ID of container's rootfs mount, created by runc in the host + // mount namespace (only set if the container does not have its own + // mount namespace). Used to unmount it once the container is destroyed. + RootfsMountID uint64 `json:"rootfs_mount_id,omitempty"` } // ID returns the container's unique ID @@ -991,6 +999,7 @@ func (c *Container) currentState() *State { IntelRdtMonPath: intelRdtMonPath, NamespacePaths: make(map[configs.NamespaceType]string), ExternalDescriptors: externalDescriptors, + RootfsMountID: c.rootfsMountID, } if pid > 0 { for _, ns := range c.config.Namespaces { diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index f1c223e9816..6a0c7576b09 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -139,6 +139,7 @@ func Load(root, id string) (*Container, error) { intelRdtManager: intelrdt.NewManager(&state.Config, id, state.IntelRdtPath), stateDir: stateDir, created: state.Created, + rootfsMountID: state.RootfsMountID, } c.state = &loadedState{c: c} if err := c.refreshState(); err != nil { diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 740064da91e..b52d3c45303 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -1014,6 +1014,12 @@ func (p *initProcess) start() (retErr error) { return err } case procHooks: + // At this point, the container rootfs is mounted. In case + // this is done in the host mount namespace, remember the + // mount, so it can be unmounted on container destroy. + if !p.config.Config.Namespaces.Contains(configs.NEWNS) { + p.container.rootfsMountID = mountID(p.config.Config.Rootfs) + } // Setup cgroup before prestart hook, so that the prestart hook could apply cgroup permissions. if err := p.manager.Set(p.config.Config.Cgroups.Resources); err != nil { return fmt.Errorf("error setting cgroup config for procHooks process: %w", err) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 446fd8c5803..6f20a654ad8 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -1097,19 +1097,75 @@ func rootfsParentMountPropagation(path string, rootPropagation int) error { } func prepareRoot(config *configs.Config) error { - flag := unix.MS_SLAVE | unix.MS_REC - if config.RootPropagation != 0 { - flag = config.RootPropagation + hostMntns := !config.Namespaces.Contains(configs.NEWNS) + // In the host mount namespace, the propagation of existing mounts + // (except for the rootfs one, see below) must not be changed, as this + // would affect the host. + if !hostMntns { + flag := unix.MS_SLAVE | unix.MS_REC + if config.RootPropagation != 0 { + flag = config.RootPropagation + } + if err := mount("", "/", "", uintptr(flag), ""); err != nil { + return err + } + + if err := rootfsParentMountPropagation(config.Rootfs, config.RootPropagation); err != nil { + return err + } + } else { + // If rootfs is a shared mount, the rootfs mount we create below + // is propagated to its peers. If any of these peers is the mount + // rootfs is mounted on (e.g. rootfs is bind mounted onto itself + // on a shared mount), the copy gets tucked under the rootfs mount, + // and unmounting our mount on destroy unmounts the rootfs mount, + // too. Make rootfs a slave to prevent that. EINVAL means rootfs + // is not a mount point, which is fine. + if err := mount("", config.Rootfs, "", unix.MS_SLAVE, ""); err != nil && !errors.Is(err, unix.EINVAL) { + return err + } } - if err := mount("", "/", "", uintptr(flag), ""); err != nil { + + if err := mount(config.Rootfs, config.Rootfs, "bind", unix.MS_BIND|unix.MS_REC, ""); err != nil { return err } + if hostMntns { + // Make the new rootfs mount a slave, so that container mounts + // do not propagate to the host (and other mount namespaces). + return mount("", config.Rootfs, "", unix.MS_SLAVE|unix.MS_REC, "") + } + return nil +} - if err := rootfsParentMountPropagation(config.Rootfs, config.RootPropagation); err != nil { - return err +// mountID returns the ID (as in /proc/self/mountinfo) of the mount the path +// resides on, or 0 if it can not be obtained. +func mountID(path string) uint64 { + var st unix.Statx_t + err := unix.Statx(unix.AT_FDCWD, path, unix.AT_SYMLINK_NOFOLLOW, unix.STATX_MNT_ID, &st) + if err != nil || st.Mask&unix.STATX_MNT_ID == 0 { + return 0 } + return st.Mnt_id +} - return mount(config.Rootfs, config.Rootfs, "bind", unix.MS_BIND|unix.MS_REC, "") +// unmountRootfs unmounts the container rootfs mount created by runc in the +// host mount namespace (see prepareRoot), together with all the container +// mounts under it. It is only done if the rootfs mount is still the one +// created by runc, so that some other mount is never unmounted. +func unmountRootfs(c *Container) { + if c.rootfsMountID == 0 { + return + } + rootfs := c.config.Rootfs + if mountID(rootfs) != c.rootfsMountID { + logrus.Debugf("not unmounting %s: mount ID mismatch", rootfs) + return + } + if err := unmount(rootfs, unix.MNT_DETACH); err != nil { + logrus.Warn(err) + return + } + c.rootfsMountID = 0 } func setReadonly() error { diff --git a/libcontainer/state_linux.go b/libcontainer/state_linux.go index 1f758e2a301..9d6e20e174d 100644 --- a/libcontainer/state_linux.go +++ b/libcontainer/state_linux.go @@ -61,6 +61,7 @@ func destroy(c *Container) error { return fmt.Errorf("unable to remove container state dir: %w", err) } c.initProcess = nil + unmountRootfs(c) err := runPoststopHooks(c) c.state = &stoppedState{c: c} return err diff --git a/tests/integration/host-mntns.bats b/tests/integration/host-mntns.bats index f05c707e29d..4ba5ee52c3f 100644 --- a/tests/integration/host-mntns.bats +++ b/tests/integration/host-mntns.bats @@ -5,25 +5,38 @@ load helpers function setup() { requires root setup_busybox + update_config ' .linux.namespaces -= [{"type": "mount"}] + | .linux.maskedPaths = [] + | .linux.readonlyPaths = [] + | .root.readonly = false' } function teardown() { - [ ! -v ROOT ] && return 0 # nothing to teardown + # Remove the rootfs mount made by a test. + if [ -v ROOT ] && mountpoint -q "$ROOT"/bundle/rootfs; then + umount -R --lazy "$ROOT"/bundle/rootfs + fi + teardown_bundle +} - # XXX runc does not unmount a container which - # shares mount namespace with the host. - umount -R --lazy "$ROOT"/bundle/rootfs +# This test goes first, since without the fix, any test which runs +# a container in the host mount namespace changes the host mounts +# propagation, so the bug would go unnoticed. +@test "runc run [host mount ns] must not change host mounts propagation" { + update_config '.process.args = ["true"]' - teardown_bundle + # Check / and the mount the container bundle resides on. + before=$(findmnt -n -o TARGET,PROPAGATION / && findmnt -n -o TARGET,PROPAGATION -T "$ROOT/bundle") + run -0 runc run test_host_mntns + after=$(findmnt -n -o TARGET,PROPAGATION / && findmnt -n -o TARGET,PROPAGATION -T "$ROOT/bundle") + echo "before: $before" + echo "after: $after" + [ "$before" = "$after" ] } @test "runc run [host mount ns + hooks]" { update_config ' .process.args = ["/bin/echo", "Hello World"] - | .hooks |= . + {"createRuntime": [{"path": "/bin/sh", "args": ["/bin/sh", "-c", "touch createRuntimeHook.$$"]}]} - | .linux.namespaces -= [{"type": "mount"}] - | .linux.maskedPaths = [] - | .linux.readonlyPaths = [] - | .root.readonly = false' + | .hooks |= . + {"createRuntime": [{"path": "/bin/sh", "args": ["/bin/sh", "-c", "touch createRuntimeHook.$$"]}]}' run -0 runc run test_host_mntns run -0 runc delete -f test_host_mntns @@ -31,3 +44,41 @@ function teardown() { run -0 ls createRuntimeHook.* [ "$(echo "$output" | wc -w)" -eq 1 ] } + +# https://github.com/opencontainers/runc/issues/2095 +@test "runc delete [host mount ns] unmounts container mounts" { + run -0 runc run -d --console-socket "$CONSOLE_SOCKET" test_host_mntns + testcontainer test_host_mntns running + + # Container rootfs and its mounts are visible on the host. + mountpoint -q rootfs + mountpoint -q rootfs/proc + + run -0 runc delete -f test_host_mntns + run ! mountpoint -q rootfs/proc + run ! mountpoint -q rootfs +} + +@test "runc run [host mount ns] unmounts container mounts on failure" { + update_config '.hooks |= . + {"createRuntime": [{"path": "/bin/false"}]}' + + run ! runc run test_host_mntns + run ! mountpoint -q rootfs/proc + run ! mountpoint -q rootfs +} + +@test "runc delete [host mount ns] keeps rootfs mounted by user" { + # The rootfs is a mount point before the container is created + # (and, if the bundle resides on a shared mount, it is a peer + # of that mount). + mount --bind rootfs rootfs + + run -0 runc run -d --console-socket "$CONSOLE_SOCKET" test_host_mntns + testcontainer test_host_mntns running + mountpoint -q rootfs/proc + + run -0 runc delete -f test_host_mntns + run ! mountpoint -q rootfs/proc + # The user mount must be kept intact. + mountpoint -q rootfs +}