From 0f99754ed7db1479d91abf5b1be80736bb9049ac Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 12 Nov 2021 12:11:58 -0800 Subject: [PATCH] runc run: fix ro /dev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit fb4c27c4b79c7d (went into v1.0.0-rc93) fixed a bug with read-only tmpfs, but introduced a bug with read-only /dev. This happens because /dev is a tmpfs mount and is therefore remounted read-only a bit earlier than before. To fix, 1. Revert the part of the above commit which remounts all tmpfs mounts as read-only in mountToRootfs. 2. Reuse finalizeRootfs (which is already used to remount /dev read-only) to also remount all ro tmpfs mounts that were previously mounted rw in mountPropagate. 3. Remove the break in finalizeRootfs, as now we have more than one mount to care about. 4. Reorder the if statements in finalizeRootfs to perform the fast check (for ro flag) first, and compare the strings second. Since /dev is most probably also a tmpfs mount, do the m.Device check first. Add a test case to validate the fix and prevent future regressions; make sure it fails before the fix: ✗ runc run [ro /dev mount] (in test file tests/integration/mounts.bats, line 45) `[ "$status" -eq 0 ]' failed runc spec (status=0): runc run test_busybox (status=1): time="2021-11-12T12:19:48-08:00" level=error msg="runc run failed: unable to start container process: error during container init: error mounting \"devpts\" to rootfs at \"/dev/pts\": mkdir /tmp/bats-run-VJXQk7/runc.0Fj70w/bundle/rootfs/dev/pts: read-only file system" Fixes: fb4c27c4b79c7d Signed-off-by: Kir Kolyshkin (cherry picked from commit 356c1712a0f44f784ce5930342964450dbd6accb) Signed-off-by: Kir Kolyshkin --- libcontainer/rootfs_linux.go | 21 +++++++-------------- tests/integration/mounts.bats | 10 ++++++++++ 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 44befb9f7a9..5fa9df0b9f5 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -153,15 +153,14 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig) (err error) { // finalizeRootfs sets anything to ro if necessary. You must call // prepareRootfs first. func finalizeRootfs(config *configs.Config) (err error) { - // remount dev as ro if specified + // Remount /dev and tmpfs mounts as ro if specified. for _, m := range config.Mounts { - if utils.CleanPath(m.Destination) == "/dev" { - if m.Flags&unix.MS_RDONLY == unix.MS_RDONLY { + if m.Flags&unix.MS_RDONLY == unix.MS_RDONLY { + if m.Device == "tmpfs" || utils.CleanPath(m.Destination) == "/dev" { if err := remountReadonly(m); err != nil { return newSystemErrorWithCausef(err, "remounting %q as readonly", m.Destination) } } - break } } @@ -431,12 +430,6 @@ func mountToRootfs(m *configs.Mount, c *mountConfig) error { return err } } - // Initially mounted rw in mountPropagate, remount to ro if flag set. - if m.Flags&unix.MS_RDONLY != 0 { - if err := remount(m, rootfs); err != nil { - return err - } - } return nil case "bind": if err := prepareBindMount(m, rootfs); err != nil { @@ -1046,10 +1039,10 @@ func mountPropagate(m *configs.Mount, rootfs string, mountLabel string) error { flags = m.Flags ) // Delay mounting the filesystem read-only if we need to do further - // operations on it. We need to set up files in "/dev" and tmpfs mounts may - // need to be chmod-ed after mounting. The mount will be remounted ro later - // in finalizeRootfs() if necessary. - if utils.CleanPath(m.Destination) == "/dev" || m.Device == "tmpfs" { + // operations on it. We need to set up files in "/dev", and other tmpfs + // mounts may need to be chmod-ed after mounting. These mounts will be + // remounted ro later in finalizeRootfs(), if necessary. + if m.Device == "tmpfs" || utils.CleanPath(m.Destination) == "/dev" { flags &= ^unix.MS_RDONLY } diff --git a/tests/integration/mounts.bats b/tests/integration/mounts.bats index 5d1ba0974ef..1ec675aca55 100644 --- a/tests/integration/mounts.bats +++ b/tests/integration/mounts.bats @@ -38,6 +38,16 @@ function teardown() { [[ "${lines[0]}" == *'ro,'* ]] } +# https://github.com/opencontainers/runc/issues/3248 +@test "runc run [ro /dev mount]" { + update_config ' .mounts |= map((select(.destination == "/dev") | .options += ["ro"]) // .) + | .process.args |= ["grep", "^tmpfs /dev", "/proc/mounts"]' + + runc run test_busybox + [ "$status" -eq 0 ] + [[ "${lines[0]}" == *'ro,'* ]] +} + # https://github.com/opencontainers/runc/issues/2683 @test "runc run [tmpfs mount with absolute symlink]" { # in container, /conf -> /real/conf