From 3640499a882db9d0de8ab002fd9b638fee62e772 Mon Sep 17 00:00:00 2001 From: Kailun Qin Date: Wed, 30 Jun 2021 06:47:17 -0400 Subject: [PATCH 1/3] libct/rootfs: consolidate utils imports Signed-off-by: Kailun Qin (cherry picked from commit c508a7bc0a0e67e4df4aa94e43ddd07d0c44e49b) Signed-off-by: Kir Kolyshkin --- libcontainer/rootfs_linux.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 430f490de..44befb9f7 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -22,7 +22,6 @@ import ( "github.com/opencontainers/runc/libcontainer/devices" "github.com/opencontainers/runc/libcontainer/userns" "github.com/opencontainers/runc/libcontainer/utils" - libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils" "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/selinux/go-selinux/label" "github.com/sirupsen/logrus" @@ -42,7 +41,7 @@ type mountConfig struct { // needsSetupDev returns true if /dev needs to be set up. func needsSetupDev(config *configs.Config) bool { for _, m := range config.Mounts { - if m.Device == "bind" && libcontainerUtils.CleanPath(m.Destination) == "/dev" { + if m.Device == "bind" && utils.CleanPath(m.Destination) == "/dev" { return false } } @@ -156,7 +155,7 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig) (err error) { func finalizeRootfs(config *configs.Config) (err error) { // remount dev as ro if specified for _, m := range config.Mounts { - if libcontainerUtils.CleanPath(m.Destination) == "/dev" { + if utils.CleanPath(m.Destination) == "/dev" { if m.Flags&unix.MS_RDONLY == unix.MS_RDONLY { if err := remountReadonly(m); err != nil { return newSystemErrorWithCausef(err, "remounting %q as readonly", m.Destination) @@ -1050,7 +1049,7 @@ func mountPropagate(m *configs.Mount, rootfs string, mountLabel string) error { // 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 libcontainerUtils.CleanPath(m.Destination) == "/dev" || m.Device == "tmpfs" { + if utils.CleanPath(m.Destination) == "/dev" || m.Device == "tmpfs" { flags &= ^unix.MS_RDONLY } From e802cfae03e3280513fa4e90e2c5b047650b7f2d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 12 Nov 2021 12:10:46 -0800 Subject: [PATCH 2/3] test/int/mount.bats: refer to github issue Signed-off-by: Kir Kolyshkin (cherry picked from commit f252eb543652dda4f47a3c183ac15d68244cd746) Signed-off-by: Kir Kolyshkin --- tests/integration/mounts.bats | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/mounts.bats b/tests/integration/mounts.bats index f21136b60..5d1ba0974 100644 --- a/tests/integration/mounts.bats +++ b/tests/integration/mounts.bats @@ -23,6 +23,7 @@ function teardown() { [[ "${lines[0]}" == *'/tmp/bind/config.json'* ]] } +# https://github.com/opencontainers/runc/issues/2246 @test "runc run [ro tmpfs mount]" { update_config ' .mounts += [{ source: "tmpfs", From cbb236757162414755420c2b3892e65a3163217c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 12 Nov 2021 12:11:58 -0800 Subject: [PATCH 3/3] 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 b247cd392a833b26751b946c4dec35126c40ec8b) Signed-off-by: Kir Kolyshkin --- libcontainer/rootfs_linux.go | 29 ++++++++++++----------------- tests/integration/mounts.bats | 10 ++++++++++ 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 44befb9f7..cff3d922d 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -153,15 +153,16 @@ 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 + // All tmpfs mounts and /dev were previously mounted as rw + // by mountPropagate. Remount them read-only as requested. for _, m := range config.Mounts { - if utils.CleanPath(m.Destination) == "/dev" { - if m.Flags&unix.MS_RDONLY == unix.MS_RDONLY { - if err := remountReadonly(m); err != nil { - return newSystemErrorWithCausef(err, "remounting %q as readonly", m.Destination) - } + if m.Flags&unix.MS_RDONLY != unix.MS_RDONLY { + continue + } + 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 +432,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 +1041,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 5d1ba0974..1ec675aca 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