From 15e5af3a07eb3f113e1522f55e44855d953041bc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 27 Oct 2017 00:21:41 -0700 Subject: [PATCH 1/2] Fix user mount /dev/shm size Commit 7120976d74195 ("Implement none, private, and shareable ipc modes") introduces a bug: if a user-specified mount for /dev/shm is provided, its size is overriden by value of ShmSize. A reproducer is simple: docker run --rm --mount type=tmpfs,dst=/dev/shm,tmpfs-size=100K \ alpine df /dev/shm This commit is an attempt to fix the bug, as well as optimize things a but and make the code easier to read. https://github.com/moby/moby/issues/35271 Signed-off-by: Kir Kolyshkin (cherry picked from commit 31d30a985d99a0eef92116a22159727f5c332784) --- components/engine/daemon/oci_linux.go | 37 +++++++++++++++------------ 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/components/engine/daemon/oci_linux.go b/components/engine/daemon/oci_linux.go index 9cf6674dfe..6917b48414 100644 --- a/components/engine/daemon/oci_linux.go +++ b/components/engine/daemon/oci_linux.go @@ -495,22 +495,35 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c userMounts[m.Destination] = struct{}{} } - // Filter out mounts from spec - noIpc := c.HostConfig.IpcMode.IsNone() + // Copy all mounts from spec to defaultMounts, except for + // - mounts overriden by a user supplied mount; + // - all mounts under /dev if a user supplied /dev is present; + // - /dev/shm, in case IpcMode is none. + // While at it, also + // - set size for /dev/shm from shmsize. var defaultMounts []specs.Mount _, mountDev := userMounts["/dev"] for _, m := range s.Mounts { - // filter out /dev/shm mount if case IpcMode is none - if noIpc && m.Destination == "/dev/shm" { + if _, ok := userMounts[m.Destination]; ok { + // filter out mount overridden by a user supplied mount continue } - // filter out mount overridden by a user supplied mount - if _, ok := userMounts[m.Destination]; !ok { - if mountDev && strings.HasPrefix(m.Destination, "/dev/") { + if mountDev && strings.HasPrefix(m.Destination, "/dev/") { + // filter out everything under /dev if /dev is user-mounted + continue + } + + if m.Destination == "/dev/shm" { + if c.HostConfig.IpcMode.IsNone() { + // filter out /dev/shm for "none" IpcMode continue } - defaultMounts = append(defaultMounts, m) + // set size for /dev/shm mount from spec + sizeOpt := "size=" + strconv.FormatInt(c.HostConfig.ShmSize, 10) + m.Options = append(m.Options, sizeOpt) } + + defaultMounts = append(defaultMounts, m) } s.Mounts = defaultMounts @@ -604,14 +617,6 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c s.Linux.MaskedPaths = nil } - // Set size for /dev/shm mount that comes from spec (IpcMode: private only) - for i, m := range s.Mounts { - if m.Destination == "/dev/shm" { - sizeOpt := "size=" + strconv.FormatInt(c.HostConfig.ShmSize, 10) - s.Mounts[i].Options = append(s.Mounts[i].Options, sizeOpt) - } - } - // TODO: until a kernel/mount solution exists for handling remount in a user namespace, // we must clear the readonly flag for the cgroups mount (@mrunalp concurs) if uidMap := daemon.idMappings.UIDs(); uidMap != nil || c.HostConfig.Privileged { From 3406769872b017af1141a2ec2e655cac1b525790 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Sun, 12 Nov 2017 18:27:05 -0800 Subject: [PATCH 2/2] integration: test case for #35271 This test case is checking that the built-in default size for /dev/shm (which is used for `--ipcmode` being `private` or `shareable`) is not overriding the size of user-defined tmpfs mount for /dev/shm. In other words, this is a regression test case for issue #35271, https://github.com/moby/moby/issues/35271 Signed-off-by: Kir Kolyshkin (cherry picked from commit 2e0a98b605fa278ee1f348c68fe7e07aed57b834) Signed-off-by: Kir Kolyshkin --- components/engine/daemon/daemon_linux_test.go | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/components/engine/daemon/daemon_linux_test.go b/components/engine/daemon/daemon_linux_test.go index c7d5117195..36ef52e25a 100644 --- a/components/engine/daemon/daemon_linux_test.go +++ b/components/engine/daemon/daemon_linux_test.go @@ -5,6 +5,13 @@ package daemon import ( "strings" "testing" + + containertypes "github.com/docker/docker/api/types/container" + "github.com/docker/docker/container" + "github.com/docker/docker/oci" + "github.com/docker/docker/pkg/idtools" + + "github.com/stretchr/testify/assert" ) const mountsFixture = `142 78 0:38 / / rw,relatime - aufs none rw,si=573b861da0b3a05b,dio @@ -102,3 +109,51 @@ func TestNotCleanupMounts(t *testing.T) { t.Fatal("Expected not to clean up /dev/shm") } } + +// TestTmpfsDevShmSizeOverride checks that user-specified /dev/tmpfs mount +// size is not overriden by the default shmsize (that should only be used +// for default /dev/shm (as in "shareable" and "private" ipc modes). +// https://github.com/moby/moby/issues/35271 +func TestTmpfsDevShmSizeOverride(t *testing.T) { + size := "777m" + mnt := "/dev/shm" + + d := Daemon{ + idMappings: &idtools.IDMappings{}, + } + c := &container.Container{ + HostConfig: &containertypes.HostConfig{ + ShmSize: 48 * 1024, // size we should NOT end up with + }, + } + ms := []container.Mount{ + { + Source: "tmpfs", + Destination: mnt, + Data: "size=" + size, + }, + } + + // convert ms to spec + spec := oci.DefaultSpec() + err := setMounts(&d, &spec, c, ms) + assert.NoError(t, err) + + // Check the resulting spec for the correct size + found := false + for _, m := range spec.Mounts { + if m.Destination == mnt { + for _, o := range m.Options { + if !strings.HasPrefix(o, "size=") { + continue + } + t.Logf("%+v\n", m.Options) + assert.Equal(t, "size="+size, o) + found = true + } + } + } + if !found { + t.Fatal("/dev/shm not found in spec, or size option missing") + } +}