Merge pull request #7026 from zyga/fix/base-migration-v2

 cmd,tests: forcibly discard mount namespace when bases change

4 files changed, 127 insertions, 21 deletions

diff --git a/cmd/snap-confine/ns-support.c b/cmd/snap-confine/ns-support.c
index 3a152f2836..672f18b6a0 100644
--- a/cmd/snap-confine/ns-support.c
+++ b/cmd/snap-confine/ns-support.c
@@ -41,6 +41,7 @@
 #include "../libsnap-confine-private/cgroup-freezer-support.h"
 #include "../libsnap-confine-private/classic.h"
 #include "../libsnap-confine-private/cleanup-funcs.h"
+#include "../libsnap-confine-private/infofile.h"
 #include "../libsnap-confine-private/locking.h"
 #include "../libsnap-confine-private/mountinfo.h"
 #include "../libsnap-confine-private/string-utils.h"
@@ -288,10 +289,71 @@ static bool should_discard_current_ns(dev_t base_snap_dev)
 }
 
 enum sc_discard_vote {
+	/**
+ * SC_DISCARD_NO denotes that the mount namespace doesn't have to be
+ * discarded. This happens when the base snap has not changed.
+ **/
	SC_DISCARD_NO = 1,
-	SC_DISCARD_YES = 2,
+	/**
+ * SC_DISCARD_SHOULD indicates that the mount namespace should be discarded
+ * but may be reused if it is still inhabited by processes. This only
+ * happens when the base snap revision changes but the name of the base
+ * snap is the same as before.
+ **/
+	SC_DISCARD_SHOULD = 2,
+	/**
+ * SC_DISCARD_MUST indicates that the mount namespace must be discarded
+ * even if it still inhabited by processes. This only happens when the name
+ * of the base snap changes.
+ **/
+	SC_DISCARD_MUST = 3,
 };
 
+/**
+ * is_base_transition returns true if a base transition is occurring.
+ *
+ * The function inspects /run/snapd/ns/snap.$SNAP_INSTANCE_NAME.info as well
+ * as the invocation parameters of snap-confine. If the base snap name, as
+ * encoded in the info file and as described by the invocation parameters
+ * differ then a base transition is occurring. If the info file is absent or
+ * does not record the name of the base snap then transition cannot be
+ * detected.
+**/
+static bool is_base_transition(const sc_invocation * inv)
+{
+	char info_path[PATH_MAX] = { 0 };
+	sc_must_snprintf(info_path,
+ sizeof info_path,
+ "/run/snapd/ns/snap.%s.info", inv->snap_instance);
+
+	FILE *stream SC_CLEANUP(sc_cleanup_file) = NULL;
+	stream = fopen(info_path, "r");
+	if (stream == NULL && errno == ENOENT) {
+	// If the info file is absent then we cannot decide if a transition had
+	// occurred. For people upgrading from snap-confine without the info
+	// file, that is the best we can do.
+	return false;
+	}
+	if (stream == NULL) {
+	die("cannot open %s", info_path);
+	}
+
+	char *base_snap_name SC_CLEANUP(sc_cleanup_string) = NULL;
+	sc_error *err = NULL;
+	if (sc_infofile_get_key
+ (stream, "base-snap-name", &base_snap_name, &err) < 0) {
+	sc_die_on_error(err);
+	}
+
+	if (base_snap_name == NULL) {
+	// If the info file doesn't record the name of the base snap then,
+	// again, we cannot decide if a transition had occurred.
+	return false;
+	}
+
+	return !sc_streq(inv->orig_base_snap_name, base_snap_name);
+}
+
 // The namespace may be stale. To check this we must actually switch into it
 // but then we use up our setns call (the kernel misbehaves if we setns twice).
 // To work around this we'll fork a child and use it to probe. The child will
@@ -365,21 +427,34 @@ static int sc_inspect_and_maybe_discard_stale_ns(int mnt_fd,
	die("cannot join preserved mount namespace");
	}
	// Check if the namespace needs to be discarded.
-	//
+	eventfd_t value = SC_DISCARD_NO;
+	const char *value_str = "no";
+
	// TODO: enable this for core distributions. This is complex because on
	// core the rootfs is mounted in initrd and is _not_ changed (no
	// pivot_root) and the base snap is again mounted (2nd time) by
	// systemd. This makes us end up in a situation where the outer base
	// snap will never match the rootfs inside the mount namespace.
-	bool should_discard =
- inv->is_normal_mode ?
- should_discard_current_ns(base_snap_dev) : false;
-
-	// Send this back to the parent: 2 - discard, 1 - keep.
+	if (inv->is_normal_mode
+ && should_discard_current_ns(base_snap_dev)) {
+	value = SC_DISCARD_SHOULD;
+	value_str = "should";
+
+	// The namespace is stale so also check if we must discard it due to the
+	// base snap changing. If the base snap changed, we must discard since even
+	// though currently running processes from this snap will continue to see
+	// the old base, we want new processes to use the new base. See LP:
+	// #1819875 for details.
+	if (is_base_transition(inv)) {
+	// The base snap has changed. We must discard ...
+	value = SC_DISCARD_MUST;
+	value_str = "must";
+	}
+	}
+	// Send this back to the parent: 3 - force discard 2 - prefer discard, 1 - keep.
	// Note that we cannot just use 0 and 1 because of the semantics of eventfd(2).
-	if (eventfd_write(event_fd, should_discard ?
- SC_DISCARD_YES : SC_DISCARD_NO) < 0) {
-	die("cannot send information to %s preserved mount namespace", should_discard ? "discard" : "keep");
+	if (eventfd_write(event_fd, value) < 0) {
+	die("cannot send information to %s preserved mount namespace", value_str);
	}
	// Exit, we're done.
	exit(0);
@@ -406,19 +481,25 @@ static int sc_inspect_and_maybe_discard_stale_ns(int mnt_fd,
	die("support process for mount namespace inspection exited abnormally");
	}
	// If the namespace is up-to-date then we are done.
-	if (value == SC_DISCARD_NO) {
-	debug("preserved mount namespace can be reused");
-	return 0;
-	}
-	// The namespace is stale, let's check if we can discard it.
-	if (sc_cgroup_freezer_occupied(inv->snap_instance)) {
-	// Some processes are still using the namespace so we cannot discard it
-	// as that would fracture the view that the set of processes inside
-	// have on what is mounted.
-	debug("preserved mount namespace is stale but occupied");
+	switch (value) {
+	case SC_DISCARD_NO:
+	debug("preserved mount is not stale, reusing");
	return 0;
+	case SC_DISCARD_SHOULD:
+	if (sc_cgroup_freezer_occupied(inv->snap_instance)) {
+	// Some processes are still using the namespace so we cannot discard it
+	// as that would fracture the view that the set of processes inside
+	// have on what is mounted.
+	debug
+ ("preserved mount namespace is stale but occupied, reusing");
+	return 0;
+	}
+	break;
+	case SC_DISCARD_MUST:
+	debug
+ ("preserved mount namespace is stale and base snap has changed, discarding");
+	break;
	}
-	// The namespace is both stale and empty. We can discard it now.
	sc_call_snap_discard_ns(snap_discard_ns_fd, inv->snap_instance);
	return EAGAIN;
 }
@@ -785,3 +866,23 @@ void sc_wait_for_helper(struct sc_mount_ns *group)
	sc_message_capture_helper(group, HELPER_CMD_EXIT);
	sc_wait_for_capture_helper(group);
 }
+
+void sc_store_ns_info(const sc_invocation * inv)
+{
+	FILE *stream SC_CLEANUP(sc_cleanup_file) = NULL;
+	char info_path[PATH_MAX] = { 0 };
+	sc_must_snprintf(info_path, sizeof info_path,
+ "/run/snapd/ns/snap.%s.info", inv->snap_instance);
+	stream = fopen(info_path, "w");
+	if (stream == NULL) {
+	die("cannot open %s", info_path);
+	}
+	fprintf(stream, "base-snap-name=%s\n", inv->orig_base_snap_name);
+	if (ferror(stream) != 0) {
+	die("I/O error when writing to %s", info_path);
+	}
+	if (fflush(stream) == EOF) {
+	die("cannot flush %s", info_path);
+	}
+	debug("saved mount namespace meta-data to %s", info_path);
+}
diff --git a/cmd/snap-confine/ns-support.h b/cmd/snap-confine/ns-support.h
index 4ef32f11fd..0f77c0674b 100644
--- a/cmd/snap-confine/ns-support.h
+++ b/cmd/snap-confine/ns-support.h
@@ -146,4 +146,6 @@ void sc_preserve_populated_per_user_mount_ns(struct sc_mount_ns *group);
 **/
 void sc_wait_for_helper(struct sc_mount_ns *group);
 
+void sc_store_ns_info(const sc_invocation * inv);
+
 #endif
diff --git a/cmd/snap-confine/snap-confine.apparmor.in b/cmd/snap-confine/snap-confine.apparmor.in
index 9fd380c63f..ae52bf1a5a 100644
--- a/cmd/snap-confine/snap-confine.apparmor.in
+++ b/cmd/snap-confine/snap-confine.apparmor.in
@@ -361,6 +361,8 @@
 # Allow snap-confine to unmount stale mount namespaces.
 umount /run/snapd/ns/*.mnt,
 /run/snapd/ns/snap.*.fstab w,
+ # Allow snap-confine to read and write mount namespace information files.
+ /run/snapd/ns/snap.*.info rw,
 # Required to correctly unmount bound mount namespace.
 # See LP: #1735459 for details.
 umount /,
diff --git a/cmd/snap-confine/snap-confine.c b/cmd/snap-confine/snap-confine.c
index f13744e6df..ac2f0375df 100644
--- a/cmd/snap-confine/snap-confine.c
+++ b/cmd/snap-confine/snap-confine.c
@@ -622,6 +622,7 @@ static void enter_non_classic_execution_environment(sc_invocation * inv,
	die("cannot unshare the mount namespace");
	}
	sc_populate_mount_ns(aa, snap_update_ns_fd, inv);
+	sc_store_ns_info(inv);
 
	/* Preserve the mount namespace. */
	sc_preserve_populated_mount_ns(group);