File 0004-cmd-snap-update-ns-detach-unused-mount-poi... of Package snapd

Overview Repositories Revisions Requests Users Attributes Meta

File 0004-cmd-snap-update-ns-detach-unused-mount-points.patch of Package snapd (Revision def28b2bdd6ef69282e13df91f803c2d)

Currently displaying revision def28b2bdd6ef69282e13df91f803c2d , Show latest

From 4f12b51d787e996a50cb4715822f0f90ab2a4dad Mon Sep 17 00:00:00 2001
From: Zygmunt Krynicki <me@zygoon.pl>
Date: Thu, 30 May 2019 22:38:16 +0200
Subject: [PATCH 4/4] cmd/snap-update-ns: detach unused mount points

When snap-update-ns determines that a mount point is no longer needed
and should be unmounted it really tried to unmount things. This was
working because usually there were no nested mount structures present.

We were alerted that a particular layout would fail to refresh. The
layout in question is simply:

layout:
      $SNAP/bar/mount:
	bind: $SNAP/foo
      $SNAP/baz:
	bind: $SNAP/bar

That is:
 - $SNAP/bar/mount is a view into $SNAP/foo
 - $SNAP/baz is a view into $SNAP/bar
    (which contains a view into SNAP/foo)

Since layout elements use $SNAP, which contains the revision number,
each time such layout is refreshed it will really be re-created as the
old paths will no longer be used. This will create a situation where
the mount point $SNAP/baz cannot be unmounted because there's
$SNAP/baz/mount is a mount point. This can be seen by looking at the
output of snap-update-ns debug log:

desired mount profile:
    	/snap/layout-content-test/x2/foo /snap/layout-content-test/x2/bar/mount none rbind,rw,x-snapd.origin=layout 0 0
    	/snap/layout-content-test/x2/bar /snap/layout-content-test/x2/baz none rbind,rw,x-snapd.origin=layout 0 0
    current mount profile (before applying changes):
    	/snap/layout-content-test/x1/foo /snap/layout-content-test/x1/bar/mount none rbind,rw,x-snapd.origin=layout 0 0
    	/snap/layout-content-test/x1/bar /snap/layout-content-test/x1/baz none rbind,rw,x-snapd.origin=layout 0 0
    desiredIDs: map[/snap/layout-content-test/x2/bar/mount:true /snap/layout-content-test/x2/baz:true]
    reuse: map[]
    mount changes needed:
    	unmount (/snap/layout-content-test/x1/bar /snap/layout-content-test/x1/baz none rbind,rw,x-snapd.origin=layout 0 0)
    	unmount (/snap/layout-content-test/x1/foo /snap/layout-content-test/x1/bar/mount none rbind,rw,x-snapd.origin=layout 0 0)
    	mount (/snap/layout-content-test/x2/foo /snap/layout-content-test/x2/bar/mount none rbind,rw,x-snapd.origin=layout 0 0)
    	mount (/snap/layout-content-test/x2/bar /snap/layout-content-test/x2/baz none rbind,rw,x-snapd.origin=layout 0 0)
    performing mount changes:
    	 * unmount (/snap/layout-content-test/x1/bar /snap/layout-content-test/x1/baz none rbind,rw,x-snapd.origin=layout 0 0)
    umount "/snap/layout-content-test/x1/baz" (error: device or resource busy)

This is obviously something where we should have used MNT_DETACH, since
that will have the kernel handle the inner elements ahead of the outer
elements.

In fact, we should never really unmount anything as running applications
may hold open file descriptors and that would cause the mount namespace
update to fail miserably with partially-applied content changes. To
avoid this problem, whenever a mount entry, that doesn't represent a
symlink, is no longer needed it is simply detached, rather than
unmounted.

Fixes: https://bugs.launchpad.net/snapd/+bug/1831010
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
---
 cmd/snap-update-ns/change.go                  |  8 ++++++-
 cmd/snap-update-ns/change_test.go             | 22 ++++++++++++++-----
 cmd/snap-update-ns/main_test.go               |  8 +++----
 tests/regression/lp-1831010/task.yaml         | 18 +++++++++++++++
 .../test-snapd-layout/bar/mount/.keep         |  0
 .../lp-1831010/test-snapd-layout/baz/.keep    |  0
 .../lp-1831010/test-snapd-layout/bin/sh       |  2 ++
 .../lp-1831010/test-snapd-layout/foo/.keep    |  0
 .../test-snapd-layout/meta/snap.yaml          | 10 +++++++++
 9 files changed, 57 insertions(+), 11 deletions(-)
 create mode 100644 tests/regression/lp-1831010/task.yaml
 create mode 100644 tests/regression/lp-1831010/test-snapd-layout/bar/mount/.keep
 create mode 100644 tests/regression/lp-1831010/test-snapd-layout/baz/.keep
 create mode 100755 tests/regression/lp-1831010/test-snapd-layout/bin/sh
 create mode 100644 tests/regression/lp-1831010/test-snapd-layout/foo/.keep
 create mode 100644 tests/regression/lp-1831010/test-snapd-layout/meta/snap.yaml

diff --git a/cmd/snap-update-ns/change.go b/cmd/snap-update-ns/change.go
index 889a813488..a4da8f4f99 100644
--- a/cmd/snap-update-ns/change.go
+++ b/cmd/snap-update-ns/change.go
@@ -495,7 +495,13 @@ func NeededChanges(currentProfile, desiredProfile *osutil.MountProfile) []*Chang
 		if reuse[current[i].Dir] {
 			changes = append(changes, &Change{Action: Keep, Entry: current[i]})
 		} else {
-			changes = append(changes, &Change{Action: Unmount, Entry: current[i]})
+			var entry osutil.MountEntry = current[i]
+			entry.Options = append([]string(nil), entry.Options...)
+			// Detach directory bind mounts and filesystem mounts.
+			if (entry.Type == "tmpfs" || entry.OptBool("bind") || entry.OptBool("rbind")) && !entry.XSnapdDetach() {
+				entry.Options = append(entry.Options, osutil.XSnapdDetach())
+			}
+			changes = append(changes, &Change{Action: Unmount, Entry: entry})
 		}
 	}
 
diff --git a/cmd/snap-update-ns/change_test.go b/cmd/snap-update-ns/change_test.go
index 9893b0e582..435b27b90f 100644
--- a/cmd/snap-update-ns/change_test.go
+++ b/cmd/snap-update-ns/change_test.go
@@ -216,18 +216,18 @@ func (s *changeSuite) TestNeededChangesTmpfsBindMountFarmUnused(c *C) {
 		{Entry: osutil.MountEntry{
 			Name:    "/var/lib/snapd/hostfs/snap/name/42/subdir/existing",
 			Dir:     "/snap/name/42/subdir/existing",
-			Options: []string{"bind", "ro", "x-snapd.needed-by=/snap/name/42/subdir", "x-snapd.synthetic"},
+			Options: []string{"bind", "ro", "x-snapd.needed-by=/snap/name/42/subdir", "x-snapd.synthetic", "x-snapd.detach"},
 		}, Action: update.Unmount},
 		{Entry: osutil.MountEntry{
 			Name:    "/snap/other/123/libs",
 			Dir:     "/snap/name/42/subdir/created",
-			Options: []string{"bind", "ro"},
+			Options: []string{"bind", "ro", "x-snapd.detach"},
 		}, Action: update.Unmount},
 		{Entry: osutil.MountEntry{
 			Name:    "tmpfs",
 			Dir:     "/snap/name/42/subdir",
 			Type:    "tmpfs",
-			Options: []string{"x-snapd.needed-by=/snap/name/42/subdir", "x-snapd.synthetic"},
+			Options: []string{"x-snapd.needed-by=/snap/name/42/subdir", "x-snapd.synthetic", "x-snapd.detach"},
 		}, Action: update.Unmount},
 	})
 }
@@ -409,9 +409,13 @@ func (s *changeSuite) TestRuntimeUsingSymlinks(c *C) {
 
 	// Let's see what the update algorithm thinks.
 	changes = update.NeededChanges(current_v1, desired_v2)
+	// e0 and e1 are like currentV1.Entries[0] and [1] but with different options.
+	currentV1Entries0 := current_v1.Entries[0]
+	currentV1Entries0.Options = append([]string(nil), currentV1Entries0.Options...)
+	currentV1Entries0.Options = append(currentV1Entries0.Options, osutil.XSnapdDetach())
 	c.Assert(changes, DeepEquals, []*update.Change{
 		// We are dropping the content interface bind mount because app changed revision
-		{Entry: current_v1.Entries[0], Action: update.Unmount},
+		{Entry: currentV1Entries0, Action: update.Unmount},
 		// We are also dropping the symlink we had in /opt/runtime
 		{Entry: current_v1.Entries[1], Action: update.Unmount},
 		// But, we are keeping the /opt tmpfs because we still want /opt/runtime to exist (neat!)
@@ -432,11 +436,17 @@ func (s *changeSuite) TestRuntimeUsingSymlinks(c *C) {
 	// again. Let's see what happens here. The desired profiles are already
 	// known so let's see what the algorithm thinks now.
 	changes = update.NeededChanges(current_v2, desired_v1)
+	currentV2Entries1 := current_v2.Entries[1]
+	currentV2Entries1.Options = append([]string(nil), currentV2Entries1.Options...)
+	currentV2Entries1.Options = append(currentV2Entries1.Options, osutil.XSnapdDetach())
+	currentV2Entries2 := current_v2.Entries[2]
+	currentV2Entries2.Options = append([]string(nil), currentV2Entries2.Options...)
+	currentV2Entries2.Options = append(currentV2Entries2.Options, osutil.XSnapdDetach())
 	c.Assert(changes, DeepEquals, []*update.Change{
 		// We are, again, dropping the content interface bind mount because app changed revision
-		{Entry: current_v2.Entries[2], Action: update.Unmount},
+		{Entry: currentV2Entries2, Action: update.Unmount},
 		// We are also dropping the bind mount from /opt/runtime since we want a symlink instead
-		{Entry: current_v2.Entries[1], Action: update.Unmount},
+		{Entry: currentV2Entries1, Action: update.Unmount},
 		// Again, we reuse the tmpfs.
 		{Entry: current_v2.Entries[0], Action: update.Keep},
 		// We are providing a symlink /opt/runtime -> to $SNAP/runtime.
diff --git a/cmd/snap-update-ns/main_test.go b/cmd/snap-update-ns/main_test.go
index 112431d22c..ce89d3feb6 100644
--- a/cmd/snap-update-ns/main_test.go
+++ b/cmd/snap-update-ns/main_test.go
@@ -191,7 +191,7 @@ func (s *mainSuite) TestRemovingSyntheticChanges(c *C) {
 				Entry: osutil.MountEntry{
 					Name: "/snap/mysnap/42/usr/share/mysnap",
 					Dir:  "/usr/share/mysnap", Type: "none",
-					Options: []string{"bind", "ro"},
+					Options: []string{"bind", "ro", "x-snapd.detach"},
 				},
 			})
 		case 1:
@@ -199,7 +199,7 @@ func (s *mainSuite) TestRemovingSyntheticChanges(c *C) {
 				Action: update.Unmount,
 				Entry: osutil.MountEntry{
 					Name: "/usr/share/awk", Dir: "/usr/share/awk", Type: "none",
-					Options: []string{"bind", "ro", "x-snapd.synthetic", "x-snapd.needed-by=/usr/share/mysnap"},
+					Options: []string{"bind", "ro", "x-snapd.synthetic", "x-snapd.needed-by=/usr/share/mysnap", "x-snapd.detach"},
 				},
 			})
 		case 2:
@@ -207,7 +207,7 @@ func (s *mainSuite) TestRemovingSyntheticChanges(c *C) {
 				Action: update.Unmount,
 				Entry: osutil.MountEntry{
 					Name: "/usr/share/adduser", Dir: "/usr/share/adduser", Type: "none",
-					Options: []string{"bind", "ro", "x-snapd.synthetic", "x-snapd.needed-by=/usr/share/mysnap"},
+					Options: []string{"bind", "ro", "x-snapd.synthetic", "x-snapd.needed-by=/usr/share/mysnap", "x-snapd.detach"},
 				},
 			})
 		case 3:
@@ -215,7 +215,7 @@ func (s *mainSuite) TestRemovingSyntheticChanges(c *C) {
 				Action: update.Unmount,
 				Entry: osutil.MountEntry{
 					Name: "tmpfs", Dir: "/usr/share", Type: "tmpfs",
-					Options: []string{"x-snapd.synthetic", "x-snapd.needed-by=/usr/share/mysnap"},
+					Options: []string{"x-snapd.synthetic", "x-snapd.needed-by=/usr/share/mysnap", "x-snapd.detach"},
 				},
 			})
 		default:
diff --git a/tests/regression/lp-1831010/task.yaml b/tests/regression/lp-1831010/task.yaml
new file mode 100644
index 0000000000..0f96ee2c56
--- /dev/null
+++ b/tests/regression/lp-1831010/task.yaml
@@ -0,0 +1,18 @@
+summary: Ensure that nested layouts can be unmounted
+details: |
+    When a snap providing a nested layout is refreshed, as a part of the
+    refresh the old mount points, corresponding to the revision being removed,
+    cannot be unmounted without first unmounting all the children. To avoid
+    this complex problem we use detach instead of unmount.
+prepare: |
+    snap pack test-snapd-layout
+restore: |
+    snap remove test-snapd-layout
+    rm -f test-snapd-layout_1_all.snap
+execute: |
+    # Install x1 and run the app to construct the mount namespace
+    snap install --dangerous test-snapd-layout_1_all.snap
+    test-snapd-layout.sh -c /bin/true
+    # Install x2 and ensure that the mount namespace allows /bin/true to execute
+    snap install --dangerous test-snapd-layout_1_all.snap
+    test-snapd-layout.sh -c /bin/true
diff --git a/tests/regression/lp-1831010/test-snapd-layout/bar/mount/.keep b/tests/regression/lp-1831010/test-snapd-layout/bar/mount/.keep
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/regression/lp-1831010/test-snapd-layout/baz/.keep b/tests/regression/lp-1831010/test-snapd-layout/baz/.keep
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/regression/lp-1831010/test-snapd-layout/bin/sh b/tests/regression/lp-1831010/test-snapd-layout/bin/sh
new file mode 100755
index 0000000000..d32c6d8994
--- /dev/null
+++ b/tests/regression/lp-1831010/test-snapd-layout/bin/sh
@@ -0,0 +1,2 @@
+#!/bin/sh
+exec /bin/sh "$@"
diff --git a/tests/regression/lp-1831010/test-snapd-layout/foo/.keep b/tests/regression/lp-1831010/test-snapd-layout/foo/.keep
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/regression/lp-1831010/test-snapd-layout/meta/snap.yaml b/tests/regression/lp-1831010/test-snapd-layout/meta/snap.yaml
new file mode 100644
index 0000000000..f2476f33d2
--- /dev/null
+++ b/tests/regression/lp-1831010/test-snapd-layout/meta/snap.yaml
@@ -0,0 +1,10 @@
+name: test-snapd-layout
+version: 1
+layout:
+  $SNAP/bar/mount:
+    bind: $SNAP/foo
+  $SNAP/baz:
+    bind: $SNAP/bar
+apps:
+  sh:
+    command: bin/sh
-- 
2.21.0

Places

File 0004-cmd-snap-update-ns-detach-unused-mount-points.patch of Package snapd (Revision def28b2bdd6ef69282e13df91f803c2d)

Places