Inconsistent --tmpfs behavior

This fix tries to address the issue raised in #22420. When
`--tmpfs` is specified with `/tmp`, the default value is
`rw,nosuid,nodev,noexec,relatime,size=65536k`. When `--tmpfs`
is specified with `/tmp:rw`, then the value changed to
`rw,nosuid,nodev,noexec,relatime`.

The reason for such an inconsistency is because docker tries
to add `size=65536k` option only when user provides no option.

This fix tries to address this issue by always pre-progating
`size=65536k` along with `rw,nosuid,nodev,noexec,relatime`.
If user provides a different value (e.g., `size=8192k`), it
will override the `size=65536k` anyway since the combined
options will be parsed and merged to remove any duplicates.

Additional test cases have been added to cover the changes
in this fix.

This fix fixes #22420.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

daemon/oci_linux.go

@@ -480,14 +480,17 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c
 		}
 
 		if m.Source == "tmpfs" {
-			opt := []string{"noexec", "nosuid", "nodev", volume.DefaultPropagationMode}
+			options := []string{"noexec", "nosuid", "nodev", volume.DefaultPropagationMode, "size=65536k"}
 			if m.Data != "" {
-				opt = append(opt, strings.Split(m.Data, ",")...)
-			} else {
-				opt = append(opt, "size=65536k")
+				options = append(options, strings.Split(m.Data, ",")...)
 			}
 
-			s.Mounts = append(s.Mounts, specs.Mount{Destination: m.Destination, Source: m.Source, Type: "tmpfs", Options: opt})
+			merged, err := mount.MergeTmpfsOptions(options)
+			if err != nil {
+				return err
+			}
+
+			s.Mounts = append(s.Mounts, specs.Mount{Destination: m.Destination, Source: m.Source, Type: "tmpfs", Options: merged})
 			continue
 		}
 

integration-cli/docker_cli_run_unix_test.go

@@ -824,6 +824,45 @@ func (s *DockerSuite) TestRunTmpfsMounts(c *check.C) {
 	}
 }
 
+// Test case for #22420
+func (s *DockerSuite) TestRunTmpfsMountsWithOptions(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+
+	expectedOptions := []string{"rw", "nosuid", "nodev", "noexec", "relatime", "size=65536k"}
+	out, _ := dockerCmd(c, "run", "--tmpfs", "/tmp", "busybox", "sh", "-c", "mount | grep 'tmpfs on /tmp'")
+	for _, option := range expectedOptions {
+		c.Assert(out, checker.Contains, option)
+	}
+
+	expectedOptions = []string{"rw", "nosuid", "nodev", "noexec", "relatime", "size=65536k"}
+	out, _ = dockerCmd(c, "run", "--tmpfs", "/tmp:rw", "busybox", "sh", "-c", "mount | grep 'tmpfs on /tmp'")
+	for _, option := range expectedOptions {
+		c.Assert(out, checker.Contains, option)
+	}
+
+	expectedOptions = []string{"rw", "nosuid", "nodev", "relatime", "size=8192k"}
+	out, _ = dockerCmd(c, "run", "--tmpfs", "/tmp:rw,exec,size=8192k", "busybox", "sh", "-c", "mount | grep 'tmpfs on /tmp'")
+	for _, option := range expectedOptions {
+		c.Assert(out, checker.Contains, option)
+	}
+
+	expectedOptions = []string{"rw", "nosuid", "nodev", "noexec", "relatime", "size=4096k"}
+	out, _ = dockerCmd(c, "run", "--tmpfs", "/tmp:rw,size=8192k,exec,size=4096k,noexec", "busybox", "sh", "-c", "mount | grep 'tmpfs on /tmp'")
+	for _, option := range expectedOptions {
+		c.Assert(out, checker.Contains, option)
+	}
+
+	// We use debian:jessie as there is no findmnt in busybox. Also the output will be in the format of
+	// TARGET PROPAGATION
+	// /tmp   shared
+	// so we only capture `shared` here.
+	expectedOptions = []string{"shared"}
+	out, _ = dockerCmd(c, "run", "--tmpfs", "/tmp:shared", "debian:jessie", "findmnt", "-o", "TARGET,PROPAGATION", "/tmp")
+	for _, option := range expectedOptions {
+		c.Assert(out, checker.Contains, option)
+	}
+}
+
 func (s *DockerSuite) TestRunSysctls(c *check.C) {
 
 	testRequires(c, DaemonIsLinux)

pkg/mount/flags.go

@@ -5,6 +5,112 @@ import (
 	"strings"
 )
 
+var flags = map[string]struct {
+	clear bool
+	flag  int
+}{
+	"defaults":      {false, 0},
+	"ro":            {false, RDONLY},
+	"rw":            {true, RDONLY},
+	"suid":          {true, NOSUID},
+	"nosuid":        {false, NOSUID},
+	"dev":           {true, NODEV},
+	"nodev":         {false, NODEV},
+	"exec":          {true, NOEXEC},
+	"noexec":        {false, NOEXEC},
+	"sync":          {false, SYNCHRONOUS},
+	"async":         {true, SYNCHRONOUS},
+	"dirsync":       {false, DIRSYNC},
+	"remount":       {false, REMOUNT},
+	"mand":          {false, MANDLOCK},
+	"nomand":        {true, MANDLOCK},
+	"atime":         {true, NOATIME},
+	"noatime":       {false, NOATIME},
+	"diratime":      {true, NODIRATIME},
+	"nodiratime":    {false, NODIRATIME},
+	"bind":          {false, BIND},
+	"rbind":         {false, RBIND},
+	"unbindable":    {false, UNBINDABLE},
+	"runbindable":   {false, RUNBINDABLE},
+	"private":       {false, PRIVATE},
+	"rprivate":      {false, RPRIVATE},
+	"shared":        {false, SHARED},
+	"rshared":       {false, RSHARED},
+	"slave":         {false, SLAVE},
+	"rslave":        {false, RSLAVE},
+	"relatime":      {false, RELATIME},
+	"norelatime":    {true, RELATIME},
+	"strictatime":   {false, STRICTATIME},
+	"nostrictatime": {true, STRICTATIME},
+}
+
+var validFlags = map[string]bool{
+	"":          true,
+	"size":      true,
+	"mode":      true,
+	"uid":       true,
+	"gid":       true,
+	"nr_inodes": true,
+	"nr_blocks": true,
+	"mpol":      true,
+}
+
+var propagationFlags = map[string]bool{
+	"bind":        true,
+	"rbind":       true,
+	"unbindable":  true,
+	"runbindable": true,
+	"private":     true,
+	"rprivate":    true,
+	"shared":      true,
+	"rshared":     true,
+	"slave":       true,
+	"rslave":      true,
+}
+
+// MergeTmpfsOptions merge mount options to make sure there is no duplicate.
+func MergeTmpfsOptions(options []string) ([]string, error) {
+	// We use collisions maps to remove duplicates.
+	// For flag, the key is the flag value (the key for propagation flag is -1)
+	// For data=value, the key is the data
+	flagCollisions := map[int]bool{}
+	dataCollisions := map[string]bool{}
+
+	var newOptions []string
+	// We process in reverse order
+	for i := len(options) - 1; i >= 0; i-- {
+		option := options[i]
+		if option == "defaults" {
+			continue
+		}
+		if f, ok := flags[option]; ok && f.flag != 0 {
+			// There is only one propagation mode
+			key := f.flag
+			if propagationFlags[option] {
+				key = -1
+			}
+			// Check to see if there is collision for flag
+			if !flagCollisions[key] {
+				// We prepend the option and add to collision map
+				newOptions = append([]string{option}, newOptions...)
+				flagCollisions[key] = true
+			}
+			continue
+		}
+		opt := strings.SplitN(option, "=", 2)
+		if len(opt) != 2 || !validFlags[opt[0]] {
+			return nil, fmt.Errorf("Invalid tmpfs option %q", opt)
+		}
+		if !dataCollisions[opt[0]] {
+			// We prepend the option and add to collision map
+			newOptions = append([]string{option}, newOptions...)
+			dataCollisions[opt[0]] = true
+		}
+	}
+
+	return newOptions, nil
+}
+
 // Parse fstab type mount options into mount() flags
 // and device specific data
 func parseOptions(options string) (int, string) {
@@ -13,45 +119,6 @@ func parseOptions(options string) (int, string) {
 		data []string
 	)
 
-	flags := map[string]struct {
-		clear bool
-		flag  int
-	}{
-		"defaults":      {false, 0},
-		"ro":            {false, RDONLY},
-		"rw":            {true, RDONLY},
-		"suid":          {true, NOSUID},
-		"nosuid":        {false, NOSUID},
-		"dev":           {true, NODEV},
-		"nodev":         {false, NODEV},
-		"exec":          {true, NOEXEC},
-		"noexec":        {false, NOEXEC},
-		"sync":          {false, SYNCHRONOUS},
-		"async":         {true, SYNCHRONOUS},
-		"dirsync":       {false, DIRSYNC},
-		"remount":       {false, REMOUNT},
-		"mand":          {false, MANDLOCK},
-		"nomand":        {true, MANDLOCK},
-		"atime":         {true, NOATIME},
-		"noatime":       {false, NOATIME},
-		"diratime":      {true, NODIRATIME},
-		"nodiratime":    {false, NODIRATIME},
-		"bind":          {false, BIND},
-		"rbind":         {false, RBIND},
-		"unbindable":    {false, UNBINDABLE},
-		"runbindable":   {false, RUNBINDABLE},
-		"private":       {false, PRIVATE},
-		"rprivate":      {false, RPRIVATE},
-		"shared":        {false, SHARED},
-		"rshared":       {false, RSHARED},
-		"slave":         {false, SLAVE},
-		"rslave":        {false, RSLAVE},
-		"relatime":      {false, RELATIME},
-		"norelatime":    {true, RELATIME},
-		"strictatime":   {false, STRICTATIME},
-		"nostrictatime": {true, STRICTATIME},
-	}
-
 	for _, o := range strings.Split(options, ",") {
 		// If the option does not exist in the flags table or the flag
 		// is not supported on the platform,
@@ -72,16 +139,6 @@ func parseOptions(options string) (int, string) {
 // ParseTmpfsOptions parse fstab type mount options into flags and data
 func ParseTmpfsOptions(options string) (int, string, error) {
 	flags, data := parseOptions(options)
-	validFlags := map[string]bool{
-		"":          true,
-		"size":      true,
-		"mode":      true,
-		"uid":       true,
-		"gid":       true,
-		"nr_inodes": true,
-		"nr_blocks": true,
-		"mpol":      true,
-	}
 	for _, o := range strings.Split(data, ",") {
 		opt := strings.SplitN(o, "=", 2)
 		if !validFlags[opt[0]] {

pkg/mount/mount_unix_test.go

@@ -137,3 +137,26 @@ func TestGetMounts(t *testing.T) {
 		t.Fatal("/ should be mounted at least")
 	}
 }
+
+func TestMergeTmpfsOptions(t *testing.T) {
+	options := []string{"noatime", "ro", "size=10k", "defaults", "atime", "defaults", "rw", "rprivate", "size=1024k", "slave"}
+	expected := []string{"atime", "rw", "size=1024k", "slave"}
+	merged, err := MergeTmpfsOptions(options)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(expected) != len(merged) {
+		t.Fatalf("Expected %s got %s", expected, merged)
+	}
+	for index := range merged {
+		if merged[index] != expected[index] {
+			t.Fatalf("Expected %s for the %dth option, got %s", expected, index, merged)
+		}
+	}
+
+	options = []string{"noatime", "ro", "size=10k", "atime", "rw", "rprivate", "size=1024k", "slave", "size"}
+	_, err = MergeTmpfsOptions(options)
+	if err == nil {
+		t.Fatal("Expected error got nil")
+	}
+}