mount: fix read-only bind (#1368)

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

container_test.go

@@ -704,7 +704,12 @@ func TestContainerExecNoBinaryExists(t *testing.T) {
 
 func TestUserNamespaces(t *testing.T) {
 	t.Parallel()
+	t.Run("WritableRootFS", func(t *testing.T) { testUserNamespaces(t, false) })
+	// see #1373 and runc#1572
+	t.Run("ReadonlyRootFS", func(t *testing.T) { testUserNamespaces(t, true) })
+}
 
+func testUserNamespaces(t *testing.T, readonlyRootFS bool) {
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -714,7 +719,7 @@ func TestUserNamespaces(t *testing.T) {
 	var (
 		image       Image
 		ctx, cancel = testContext()
-		id          = t.Name()
+		id          = strings.Replace(t.Name(), "/", "-", -1)
 	)
 	defer cancel()
 
@@ -726,13 +731,17 @@ func TestUserNamespaces(t *testing.T) {
 		}
 	}
 
-	container, err := client.NewContainer(ctx, id,
-		WithNewSpec(withImageConfig(image),
+	opts := []NewContainerOpts{WithNewSpec(withImageConfig(image),
 		withExitStatus(7),
 		withUserNamespace(0, 1000, 10000),
-		),
-		withRemappedSnapshot(id, image, 1000, 1000),
-	)
+	)}
+	if readonlyRootFS {
+		opts = append(opts, withRemappedSnapshotView(id, image, 1000, 1000))
+	} else {
+		opts = append(opts, withRemappedSnapshot(id, image, 1000, 1000))
+	}
+
+	container, err := client.NewContainer(ctx, id, opts...)
 	if err != nil {
 		t.Error(err)
 		return

helpers_unix_test.go

@@ -42,6 +42,7 @@ func withExecArgs(s *specs.Process, args ...string) {
 var (
 	withUserNamespace        = WithUserNamespace
 	withRemappedSnapshot     = WithRemappedSnapshot
+	withRemappedSnapshotView = WithRemappedSnapshotView
 	withNewSnapshot          = WithNewSnapshot
 	withImageConfig          = WithImageConfig
 )

helpers_windows_test.go

@@ -63,6 +63,12 @@ func withRemappedSnapshot(id string, i Image, u, g uint32) NewContainerOpts {
 	}
 }
 
+func withRemappedSnapshotView(id string, i Image, u, g uint32) NewContainerOpts {
+	return func(ctx context.Context, client *Client, c *containers.Container) error {
+		return nil
+	}
+}
+
 func withNoop(_ context.Context, _ *Client, _ *containers.Container, _ *specs.Spec) error {
 	return nil
 }

mount/mount_linux.go

@@ -8,7 +8,36 @@ import (
 
 func (m *Mount) Mount(target string) error {
 	flags, data := parseMountOptions(m.Options)
-	return unix.Mount(m.Source, target, m.Type, uintptr(flags), data)
+
+	// propagation types.
+	const ptypes = unix.MS_SHARED | unix.MS_PRIVATE | unix.MS_SLAVE | unix.MS_UNBINDABLE
+
+	// Ensure propagation type change flags aren't included in other calls.
+	oflags := flags &^ ptypes
+
+	// In the case of remounting with changed data (data != ""), need to call mount (moby/moby#34077).
+	if flags&unix.MS_REMOUNT == 0 || data != "" {
+		// Initial call applying all non-propagation flags for mount
+		// or remount with changed data
+		if err := unix.Mount(m.Source, target, m.Type, uintptr(oflags), data); err != nil {
+			return err
+		}
+	}
+
+	if flags&ptypes != 0 {
+		// Change the propagation type.
+		const pflags = ptypes | unix.MS_REC | unix.MS_SILENT
+		if err := unix.Mount("", target, "", uintptr(flags&pflags), ""); err != nil {
+			return err
+		}
+	}
+
+	const broflags = unix.MS_BIND | unix.MS_RDONLY
+	if oflags&broflags == broflags {
+		// Remount the bind to apply read only.
+		return unix.Mount("", target, "", uintptr(oflags|unix.MS_REMOUNT), "")
+	}
+	return nil
 }
 
 func Unmount(mount string, flags int) error {

snapshot/testsuite/testsuite.go

@@ -38,6 +38,8 @@ func SnapshotterSuite(t *testing.T, name string, snapshotterFn func(ctx context.
 
 	// Rename test still fails on some kernels with overlay
 	//t.Run("Rename", makeTest(name, snapshotterFn, checkRename))
+
+	t.Run("ViewReadonly", makeTest(name, snapshotterFn, checkSnapshotterViewReadonly))
 }
 
 func makeTest(name string, snapshotterFn func(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error), fn func(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string)) func(t *testing.T) {
@@ -651,3 +653,40 @@ func checkRemove(ctx context.Context, t *testing.T, snapshotter snapshot.Snapsho
 		t.Fatal(err)
 	}
 }
+
+// checkSnapshotterViewReadonly ensures a KindView snapshot to be mounted as a read-only filesystem.
+// This function is called only when WithTestViewReadonly is true.
+func checkSnapshotterViewReadonly(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) {
+	preparing := filepath.Join(work, "preparing")
+	if _, err := snapshotter.Prepare(ctx, preparing, ""); err != nil {
+		t.Fatal(err)
+	}
+	committed := filepath.Join(work, "commited")
+	if err := snapshotter.Commit(ctx, committed, preparing); err != nil {
+		t.Fatal(err)
+	}
+	view := filepath.Join(work, "view")
+	m, err := snapshotter.View(ctx, view, committed)
+	if err != nil {
+		t.Fatal(err)
+	}
+	viewMountPoint := filepath.Join(work, "view-mount")
+	if err := os.MkdirAll(viewMountPoint, 0777); err != nil {
+		t.Fatal(err)
+	}
+
+	// Just checking the option string of m is not enough, we need to test real mount. (#1368)
+	if err := mount.MountAll(m, viewMountPoint); err != nil {
+		t.Fatal(err)
+	}
+
+	testfile := filepath.Join(viewMountPoint, "testfile")
+	if err := ioutil.WriteFile(testfile, []byte("testcontent"), 0777); err != nil {
+		t.Logf("write to %q failed with %v (EROFS is expected but can be other error code)", testfile, err)
+	} else {
+		t.Fatalf("write to %q should fail (EROFS) but did not fail", testfile)
+	}
+	testutil.Unmount(t, viewMountPoint)
+	assert.NoError(t, snapshotter.Remove(ctx, view))
+	assert.NoError(t, snapshotter.Remove(ctx, committed))
+}

spec_opts_unix.go

@@ -225,6 +225,15 @@ func WithUserNamespace(container, host, size uint32) SpecOpts {
 // WithRemappedSnapshot creates a new snapshot and remaps the uid/gid for the
 // filesystem to be used by a container with user namespaces
 func WithRemappedSnapshot(id string, i Image, uid, gid uint32) NewContainerOpts {
+	return withRemappedSnapshotBase(id, i, uid, gid, false)
+}
+
+// WithRemappedSnapshotView is similar to WithRemappedSnapshot but rootfs is mounted as read-only.
+func WithRemappedSnapshotView(id string, i Image, uid, gid uint32) NewContainerOpts {
+	return withRemappedSnapshotBase(id, i, uid, gid, true)
+}
+
+func withRemappedSnapshotBase(id string, i Image, uid, gid uint32, readonly bool) NewContainerOpts {
 	return func(ctx context.Context, client *Client, c *containers.Container) error {
 		diffIDs, err := i.(*image).i.RootFS(ctx, client.ContentStore())
 		if err != nil {
@@ -257,7 +266,12 @@ func WithRemappedSnapshot(id string, i Image, uid, gid uint32) NewContainerOpts
 		if err := snapshotter.Commit(ctx, usernsID, usernsID+"-remap"); err != nil {
 			return err
 		}
-		if _, err := snapshotter.Prepare(ctx, id, usernsID); err != nil {
+		if readonly {
+			_, err = snapshotter.View(ctx, id, usernsID)
+		} else {
+			_, err = snapshotter.Prepare(ctx, id, usernsID)
+		}
+		if err != nil {
 			return err
 		}
 		c.RootFS = id