Mount /dev/shm as shared mount between containers in pod

K8s mounts `/dev/shm` as a `bind` mount pointing to the same location for
all containers in a pod, so it can be shared amoung them. This causes
gVisor with VFS2 to mount `/dev/shm` over 9p to the gofer. This confuses
glibc which looks for a tmpfs mount for the directory to use with shm_open(3).

The shim now detects this intent and add makes the corresponding changes to
the spec to make `/dev/shm` be mounted as a pod shared tmpfs volume. It consists
of annotations to indicate that the volume is pod shared, and changes to mount
type from `bind` to `tmpfs`.

It also required changes in runsc to allow shared mounts with more restrictive
options, so it doesn't break containers that mount `/dev/shm` with less
restrictive options, like `ro`.

Closes #6943

PiperOrigin-RevId: 418035851

pkg/shim/utils/volumes.go

@@ -22,7 +22,13 @@ import (
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
-const volumeKeyPrefix = "dev.gvisor.spec.mount."
+const (
+	volumeKeyPrefix = "dev.gvisor.spec.mount."
+
+	// devshmName is the volume name used for /dev/shm. Pick a name that is
+	// unlikely to be used.
+	devshmName = "gvisorinternaldevshm"
+)
 
 var kubeletPodsDir = "/var/lib/kubelet/pods"
 
@@ -99,7 +105,7 @@ func UpdateVolumeAnnotations(s *specs.Spec) (bool, error) {
 			return false, nil
 		}
 	}
-	var updated bool
+	updated := false
 	for k, v := range s.Annotations {
 		if !isVolumeKey(k) {
 			continue
@@ -136,6 +142,67 @@ func UpdateVolumeAnnotations(s *specs.Spec) (bool, error) {
 			}
 		}
 	}
+
+	if ok, err := configureShm(s); err != nil {
+		return false, err
+	} else if ok {
+		updated = true
+	}
+
+	return updated, nil
+}
+
+// configureShm sets up annotations to mount /dev/shm as a pod shared tmpfs
+// mount inside containers.
+//
+// Pods are configured to mount /dev/shm to a common path in the host, so it's
+// shared among containers in the same pod. In gVisor, /dev/shm must be
+// converted to a tmpfs mount inside the sandbox, otherwise shm_open(3) doesn't
+// use it (see where_is_shmfs() in glibc). Mount annotation hints are used to
+// instruct runsc to mount the same tmpfs volume in all containers inside the
+// pod.
+func configureShm(s *specs.Spec) (bool, error) {
+	const (
+		shmPath    = "/dev/shm"
+		devshmType = "tmpfs"
+	)
+
+	// Some containers contain a duplicate mount entry for /dev/shm using tmpfs.
+	// If this is detected, remove the extraneous entry to ensure the correct one
+	// is used.
+	duplicate := -1
+	for i, m := range s.Mounts {
+		if m.Destination == shmPath && m.Type == devshmType {
+			duplicate = i
+			break
+		}
+	}
+
+	updated := false
+	for i := range s.Mounts {
+		m := &s.Mounts[i]
+		if m.Destination == shmPath && m.Type == "bind" {
+			if IsSandbox(s) {
+				s.Annotations["dev.gvisor.spec.mount."+devshmName+".source"] = m.Source
+				s.Annotations["dev.gvisor.spec.mount."+devshmName+".type"] = devshmType
+				s.Annotations["dev.gvisor.spec.mount."+devshmName+".share"] = "pod"
+				// Given that we don't have visibility into mount options for all
+				// containers, assume broad access for the master mount (it's tmpfs
+				// inside the sandbox anyways) and apply options to subcontainers as
+				// they bind mount individually.
+				s.Annotations["dev.gvisor.spec.mount."+devshmName+".options"] = "rw"
+			}
+
+			changeMountType(m, devshmType)
+			updated = true
+
+			// Remove the duplicate entry now that we found the shared /dev/shm mount.
+			if duplicate >= 0 {
+				s.Mounts = append(s.Mounts[:duplicate], s.Mounts[duplicate+1:]...)
+			}
+			break
+		}
+	}
 	return updated, nil
 }
 

pkg/shim/utils/volumes_test.go

@@ -307,23 +307,135 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 			},
 			expectUpdate: true,
 		},
+		{
+			name: "shm-sandbox",
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					sandboxLogDirAnnotation: testLogDirPath,
+					ContainerTypeAnnotation: containerTypeSandbox,
+				},
+				Mounts: []specs.Mount{
+					{
+						Destination: "/dev/shm",
+						Type:        "bind",
+						Source:      testVolumePath,
+						Options:     []string{"ro", "foo"},
+					},
+				},
+			},
+			expected: &specs.Spec{
+				Annotations: map[string]string{
+					sandboxLogDirAnnotation:                   testLogDirPath,
+					ContainerTypeAnnotation:                   containerTypeSandbox,
+					volumeKeyPrefix + devshmName + ".share":   "pod",
+					volumeKeyPrefix + devshmName + ".type":    "tmpfs",
+					volumeKeyPrefix + devshmName + ".options": "rw",
+					volumeKeyPrefix + devshmName + ".source":  testVolumePath,
+				},
+				Mounts: []specs.Mount{
+					{
+						Destination: "/dev/shm",
+						Type:        "tmpfs",
+						Source:      testVolumePath,
+						Options:     []string{"ro", "foo"},
+					},
+				},
+			},
+			expectUpdate: true,
+		},
+		{
+			name: "shm-container",
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					ContainerTypeAnnotation: ContainerTypeContainer,
+				},
+				Mounts: []specs.Mount{
+					{
+						Destination: "/dev/shm",
+						Type:        "bind",
+						Source:      testVolumePath,
+						Options:     []string{"ro", "foo"},
+					},
+				},
+			},
+			expected: &specs.Spec{
+				Annotations: map[string]string{
+					ContainerTypeAnnotation: ContainerTypeContainer,
+				},
+				Mounts: []specs.Mount{
+					{
+						Destination: "/dev/shm",
+						Type:        "tmpfs",
+						Source:      testVolumePath,
+						Options:     []string{"ro", "foo"},
+					},
+				},
+			},
+			expectUpdate: true,
+		},
+		{
+			name: "shm-duplicate",
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					ContainerTypeAnnotation: ContainerTypeContainer,
+				},
+				Mounts: []specs.Mount{
+					{
+						Destination: "/dev/shm",
+						Type:        "bind",
+						Source:      testVolumePath,
+						Options:     []string{"ro", "foo"},
+					},
+					{
+						Destination: "/dev/shm",
+						Type:        "tmpfs",
+					},
+					{
+						Destination: "/home",
+						Type:        "bind",
+						Source:      "/another/mount",
+						Options:     []string{"rw"},
+					},
+				},
+			},
+			expected: &specs.Spec{
+				Annotations: map[string]string{
+					ContainerTypeAnnotation: ContainerTypeContainer,
+				},
+				Mounts: []specs.Mount{
+					{
+						Destination: "/dev/shm",
+						Type:        "tmpfs",
+						Source:      testVolumePath,
+						Options:     []string{"ro", "foo"},
+					},
+					{
+						Destination: "/home",
+						Type:        "bind",
+						Source:      "/another/mount",
+						Options:     []string{"rw"},
+					},
+				},
+			},
+			expectUpdate: true,
+		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			updated, err := UpdateVolumeAnnotations(test.spec)
 			if test.expectErr {
 				if err == nil {
-					t.Fatal("Expected error, but got nil")
+					t.Fatal("UpdateVolumeAnnotations(spec): nil, want: error")
 				}
 				return
 			}
 			if err != nil {
-				t.Fatalf("Unexpected error: %v", err)
-			}
-			if !reflect.DeepEqual(test.expected, test.spec) {
-				t.Fatalf("Expected %+v, got %+v", test.expected, test.spec)
+				t.Fatalf("UpdateVolumeAnnotations(spec): %v", err)
 			}
 			if test.expectUpdate != updated {
-				t.Errorf("Expected %v, got %v", test.expected, updated)
+				t.Errorf("want: %v, got: %v", test.expectUpdate, updated)
+			}
+			if !reflect.DeepEqual(test.expected, test.spec) {
+				t.Fatalf("want: %+v, got: %+v", test.expected, test.spec)
 			}
 		})
 	}

runsc/boot/BUILD

@@ -132,6 +132,7 @@ go_test(
         "compat_test.go",
         "fs_test.go",
         "loader_test.go",
+        "vfs_test.go",
     ],
     library = ":boot",
     deps = [

runsc/boot/fs.go

@@ -492,10 +492,10 @@ func (m *mountHint) isSupported() bool {
 // For now enforce that all options are the same. Once bind mount is properly
 // supported, then we should ensure the master is less restrictive than the
 // container, e.g. master can be 'rw' while container mounts as 'ro'.
-func (m *mountHint) checkCompatible(mount *specs.Mount) error {
+func (m *mountHint) checkCompatible(replica *specs.Mount) error {
 	// Remove options that don't affect to mount's behavior.
 	masterOpts := filterUnsupportedOptions(&m.mount)
-	replicaOpts := filterUnsupportedOptions(mount)
+	replicaOpts := filterUnsupportedOptions(replica)
 
 	if len(masterOpts) != len(replicaOpts) {
 		return fmt.Errorf("mount options in annotations differ from container mount, annotation: %s, mount: %s", masterOpts, replicaOpts)
@@ -511,6 +511,25 @@ func (m *mountHint) checkCompatible(mount *specs.Mount) error {
 	return nil
 }
 
+// checkCompatibleVFS2 verifies that shared mount is compatible with master.
+// Master options must be the same or less restrictive than the container mount,
+// e.g. master can be 'rw' while container mounts as 'ro'.
+func (m *mountHint) checkCompatibleVFS2(replica *specs.Mount) error {
+	masterOpts := parseMountOptionsVFS2(m.mount.Options)
+	replicaOpts := parseMountOptionsVFS2(replica.Options)
+
+	if masterOpts.ReadOnly && !replicaOpts.ReadOnly {
+		return fmt.Errorf("cannot mount read-write shared mount because master is read-only, mount: %+v", replica)
+	}
+	if masterOpts.Flags.NoExec && !replicaOpts.Flags.NoExec {
+		return fmt.Errorf("cannot mount exec enabled shared mount because master is noexec, mount: %+v", replica)
+	}
+	if masterOpts.Flags.NoATime && !replicaOpts.Flags.NoATime {
+		return fmt.Errorf("cannot mount atime enabled shared mount because master is noatime, mount: %+v", replica)
+	}
+	return nil
+}
+
 func (m *mountHint) fileAccessType() config.FileAccessType {
 	if m.share == container {
 		return config.FileAccessExclusive

runsc/boot/vfs.go

@@ -540,29 +540,10 @@ func (c *containerMounter) getMountNameAndOptionsVFS2(conf *config.Config, m *mo
 		return "", nil, false, nil
 	}
 
-	opts := &vfs.MountOptions{
-		GetFilesystemOptions: vfs.GetFilesystemOptions{
-			Data:         strings.Join(data, ","),
-			InternalData: internalData,
-		},
-		InternalMount: true,
-	}
-
-	for _, o := range m.mount.Options {
-		switch o {
-		case "rw":
-			opts.ReadOnly = false
-		case "ro":
-			opts.ReadOnly = true
-		case "noatime":
-			opts.Flags.NoATime = true
-		case "noexec":
-			opts.Flags.NoExec = true
-		case "bind", "rbind":
-			// These are the same as a mount with type="bind".
-		default:
-			log.Warningf("ignoring unknown mount option %q", o)
-		}
+	opts := parseMountOptionsVFS2(m.mount.Options)
+	opts.GetFilesystemOptions = vfs.GetFilesystemOptions{
+		Data:         strings.Join(data, ","),
+		InternalData: internalData,
 	}
 
 	if verityRequested {
@@ -582,6 +563,30 @@ func (c *containerMounter) getMountNameAndOptionsVFS2(conf *config.Config, m *mo
 	return fsName, opts, useOverlay, nil
 }
 
+func parseMountOptionsVFS2(opts []string) *vfs.MountOptions {
+	mountOpts := &vfs.MountOptions{
+		InternalMount: true,
+	}
+	// Note: update mountHint.CheckCompatibleVFS2 when more options are added.
+	for _, o := range opts {
+		switch o {
+		case "ro":
+			mountOpts.ReadOnly = true
+		case "noatime":
+			mountOpts.Flags.NoATime = true
+		case "noexec":
+			mountOpts.Flags.NoExec = true
+		case "rw", "atime", "exec":
+			// These use the default value and don't need to be set.
+		case "bind", "rbind":
+			// These are the same as a mount with type="bind".
+		default:
+			log.Warningf("ignoring unknown mount option %q", o)
+		}
+	}
+	return mountOpts
+}
+
 func parseKeyValue(s string) (string, string, bool) {
 	tokens := strings.SplitN(s, "=", 2)
 	if len(tokens) < 2 {
@@ -759,7 +764,7 @@ func (c *containerMounter) mountSharedMasterVFS2(ctx context.Context, conf *conf
 // mountSharedSubmount binds mount to a previously mounted volume that is shared
 // among containers in the same pod.
 func (c *containerMounter) mountSharedSubmountVFS2(ctx context.Context, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, mount *specs.Mount, source *mountHint) (*vfs.Mount, error) {
-	if err := source.checkCompatible(mount); err != nil {
+	if err := source.checkCompatibleVFS2(mount); err != nil {
 		return nil, err
 	}
 

runsc/boot/vfs_test.go

@@ -0,0 +1,82 @@
+// Copyright 2021 The gVisor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package boot
+
+import (
+	"strings"
+	"testing"
+
+	specs "github.com/opencontainers/runtime-spec/specs-go"
+)
+
+func TestHintsCheckCompatible(t *testing.T) {
+	for _, tc := range []struct {
+		name        string
+		masterOpts  []string
+		replicaOpts []string
+		err         string
+	}{
+		{
+			name: "empty",
+		},
+		{
+			name:        "same",
+			masterOpts:  []string{"ro", "noatime", "noexec"},
+			replicaOpts: []string{"ro", "noatime", "noexec"},
+		},
+		{
+			name:        "compatible",
+			masterOpts:  []string{"rw", "atime", "exec"},
+			replicaOpts: []string{"ro", "noatime", "noexec"},
+		},
+		{
+			name:        "unsupported",
+			masterOpts:  []string{"nofoo", "nodev"},
+			replicaOpts: []string{"foo", "dev"},
+		},
+		{
+			name:        "incompatible-ro",
+			masterOpts:  []string{"ro"},
+			replicaOpts: []string{"rw"},
+			err:         "read-write",
+		},
+		{
+			name:        "incompatible-atime",
+			masterOpts:  []string{"noatime"},
+			replicaOpts: []string{"atime"},
+			err:         "noatime",
+		},
+		{
+			name:        "incompatible-exec",
+			masterOpts:  []string{"noexec"},
+			replicaOpts: []string{"exec"},
+			err:         "noexec",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			master := mountHint{mount: specs.Mount{Options: tc.masterOpts}}
+			replica := specs.Mount{Options: tc.replicaOpts}
+			if err := master.checkCompatibleVFS2(&replica); err != nil {
+				if !strings.Contains(err.Error(), tc.err) {
+					t.Fatalf("wrong error, want: %q, got: %q", tc.err, err)
+				}
+			} else {
+				if len(tc.err) > 0 {
+					t.Fatalf("error %q expected", tc.err)
+				}
+			}
+		})
+	}
+}

runsc/container/multi_container_test.go

@@ -101,8 +101,9 @@ func startContainers(conf *config.Config, specs []*specs.Spec, ids []string) ([]
 type execDesc struct {
 	c    *Container
 	cmd  []string
-	want int
 	name string
+	want int
+	err  string
 }
 
 func execMany(t *testing.T, conf *config.Config, execs []execDesc) {
@@ -110,9 +111,13 @@ func execMany(t *testing.T, conf *config.Config, execs []execDesc) {
 		t.Run(exec.name, func(t *testing.T) {
 			args := &control.ExecArgs{Argv: exec.cmd}
 			if ws, err := exec.c.executeSync(conf, args); err != nil {
-				t.Errorf("error executing %+v: %v", args, err)
+				if len(exec.err) == 0 || !strings.Contains(err.Error(), exec.err) {
+					t.Errorf("error executing %+v: %v", args, err)
+				}
+			} else if len(exec.err) > 0 {
+				t.Errorf("exec %q didn't fail as expected", exec.cmd)
 			} else if ws.ExitStatus() != exec.want {
-				t.Errorf("%q: exec %q got exit status: %d, want: %d", exec.name, exec.cmd, ws.ExitStatus(), exec.want)
+				t.Errorf("exec %q got exit status: %d, want: %d", exec.cmd, ws.ExitStatus(), exec.want)
 			}
 		})
 	}
@@ -1387,6 +1392,75 @@ func TestMultiContainerSharedMountReadonly(t *testing.T) {
 	}
 }
 
+// Test that pod mounts can be mounted with less restrictive options in
+// container mounts.
+func TestMultiContainerSharedMountCompatible(t *testing.T) {
+	rootDir, cleanup, err := testutil.SetupRootDir()
+	if err != nil {
+		t.Fatalf("error creating root dir: %v", err)
+	}
+	defer cleanup()
+	conf := testutil.TestConfig(t)
+	conf.RootDir = rootDir
+
+	sleep := []string{"sleep", "100"}
+	podSpec, ids := createSpecs(sleep, sleep)
+
+	// Init container and annotations allow read-write and exec.
+	mnt0 := specs.Mount{
+		Destination: "/mydir/test",
+		Source:      "/some/dir",
+		Type:        "tmpfs",
+		Options:     []string{"rw", "exec"},
+	}
+	podSpec[0].Mounts = append(podSpec[0].Mounts, mnt0)
+
+	// While subcontainer mount has more restrictive options: read-only, noexec.
+	mnt1 := mnt0
+	mnt1.Destination = "/mydir2/test2"
+	mnt1.Options = []string{"ro", "noexec"}
+	podSpec[1].Mounts = append(podSpec[1].Mounts, mnt1)
+
+	createSharedMount(mnt0, "test-mount", podSpec...)
+
+	containers, cleanup, err := startContainers(conf, podSpec, ids)
+	if err != nil {
+		t.Fatalf("error starting containers: %v", err)
+	}
+	defer cleanup()
+
+	execs := []execDesc{
+		{
+			c:    containers[1],
+			cmd:  []string{"/bin/touch", path.Join(mnt1.Destination, "fail")},
+			want: 1,
+			name: "fails write to container1",
+		},
+		{
+			c:    containers[0],
+			cmd:  []string{"/bin/cp", "/usr/bin/test", mnt0.Destination},
+			name: "writes to container0",
+		},
+		{
+			c:    containers[1],
+			cmd:  []string{"/usr/bin/test", "-f", path.Join(mnt1.Destination, "test")},
+			name: "file appears in container1",
+		},
+		{
+			c:    containers[0],
+			cmd:  []string{path.Join(mnt0.Destination, "test"), "-d", mnt0.Destination},
+			name: "container0 can execute",
+		},
+		{
+			c:    containers[1],
+			cmd:  []string{path.Join(mnt1.Destination, "test"), "-d", mnt1.Destination},
+			err:  "permission denied",
+			name: "container1 cannot execute",
+		},
+	}
+	execMany(t, conf, execs)
+}
+
 // Test that shared pod mounts continue to work after container is restarted.
 func TestMultiContainerSharedMountRestart(t *testing.T) {
 	for name, conf := range configs(t, all...) {
@@ -2088,3 +2162,54 @@ func TestDuplicateEnvVariable(t *testing.T) {
 		}
 	}
 }
+
+// Test that /dev/shm can be shared between containers.
+func TestMultiContainerShm(t *testing.T) {
+	conf := testutil.TestConfig(t)
+
+	rootDir, cleanup, err := testutil.SetupRootDir()
+	if err != nil {
+		t.Fatalf("error creating root dir: %v", err)
+	}
+	defer cleanup()
+	conf.RootDir = rootDir
+
+	sleep := []string{"sleep", "100"}
+	testSpecs, ids := createSpecs(sleep, sleep)
+
+	sharedMount := specs.Mount{
+		Destination: "/dev/shm",
+		Source:      "/some/path",
+		Type:        "tmpfs",
+	}
+
+	// Add shared /dev/shm mount to all containers.
+	for _, spec := range testSpecs {
+		spec.Mounts = append(spec.Mounts, sharedMount)
+	}
+
+	// Create annotation hints for the init container.
+	createSharedMount(sharedMount, "devshm", testSpecs[0])
+
+	containers, cleanup, err := startContainers(conf, testSpecs, ids)
+	if err != nil {
+		t.Fatalf("error starting containers: %v", err)
+	}
+	defer cleanup()
+
+	// Write file to shared /dev/shm directory in one container.
+	const output = "/dev/shm/file.txt"
+	exec0 := fmt.Sprintf("echo 123 > %s", output)
+	if ws, err := execute(conf, containers[0], "/bin/sh", "-c", exec0); err != nil || ws.ExitStatus() != 0 {
+		t.Fatalf("exec failed, ws: %v, err: %v", ws, err)
+	}
+
+	// Check that file can be found in the other container.
+	out, err := executeCombinedOutput(conf, containers[1], "/bin/cat", output)
+	if err != nil {
+		t.Fatalf("exec failed: %v", err)
+	}
+	if want := "123\n"; string(out) != want {
+		t.Fatalf("wrong output, want: %q, got: %v", want, out)
+	}
+}