WIP

cmd/libsnap-confine-private/locking-test.c

@@ -152,6 +152,54 @@ static void test_sc_enable_sanity_timeout(void)
 	    ("sanity timeout expired: Interrupted system call\n");
 }
 
+// Set alternate inhibit directory
+static void sc_set_inhibit_dir(const char *dir)
+{
+	sc_inhibit_dir = dir;
+}
+
+// Use temporary directory for inhibition.
+//
+// The directory is automatically reset to the real value at the end of the
+// test.
+static const char *sc_test_use_fake_inhibit_dir(void)
+{
+	char *inhibit_dir = g_dir_make_tmp(NULL, NULL);
+	g_assert_nonnull(inhibit_dir);
+	g_test_queue_free(inhibit_dir);
+	g_test_queue_destroy((GDestroyNotify) rm_rf_tmp, inhibit_dir);
+	g_test_queue_destroy((GDestroyNotify) sc_set_inhibit_dir, SC_INHIBIT_DIR);
+	sc_set_inhibit_dir(inhibit_dir);
+	return inhibit_dir;
+}
+
+static void test_sc_snap_is_inhibited__missing_dir(void)
+{
+	const char *d = g_dir_make_tmp(NULL, NULL);
+	g_test_queue_destroy((GDestroyNotify) rm_rf_tmp, inhibit_dir);
+
+	char subd[PATH_MAX];
+	sc_must_snprintf(subd, sizeof subd, "%s/absent", d);
+	sc_set_inhibit_dir(subd);
+	g_assert_false(sc_snap_is_inhibited("foo", SC_SNAP_HINT_INHIBITED_FOR_REMOVE));
+}
+
+static void test_sc_snap_is_inhibited__missing_file(void)
+{
+	const char *d = sc_test_use_fake_inhibit_dir();
+	g_assert_false(sc_snap_is_inhibited("foo", SC_SNAP_HINT_INHIBITED_FOR_REMOVE));
+}
+
+static void test_sc_snap_is_inhibited__present_file(void)
+{
+	const char *d = sc_test_use_fake_inhibit_dir();
+	char pname[PATH_MAX];
+	sc_must_snprintf(pname, sizeof pname, "%s/foo.remove", d);
+	g_assert_true(g_file_set_contents(pname, "", -1, NULL));
+
+	g_assert_true(sc_snap_is_inhibited("foo", SC_SNAP_HINT_INHIBITED_FOR_REMOVE));
+}
+
 static void __attribute__((constructor)) init(void)
 {
 	g_test_add_func("/locking/sc_lock_unlock", test_sc_lock_unlock);
@@ -161,4 +209,10 @@ static void __attribute__((constructor)) init(void)
 			test_sc_verify_snap_lock__locked);
 	g_test_add_func("/locking/sc_verify_snap_lock__unlocked",
 			test_sc_verify_snap_lock__unlocked);
+	g_test_add_func("/locking/sc_snap_is_inhibited__missing_dir",
+			test_sc_snap_is_inhibited__missing_dir);
+	g_test_add_func("/locking/sc_snap_is_inhibited__missing_file",
+			test_sc_snap_is_inhibited__missing_file);
+	g_test_add_func("/locking/sc_snap_is_inhibited__present_file",
+			test_sc_snap_is_inhibited__present_file);
 }

cmd/libsnap-confine-private/locking.c

@@ -206,3 +206,40 @@ void sc_unlock(int lock_fd)
 	}
 	close(lock_fd);
 }
+
+#define SC_INHIBIT_DIR "/var/lib/snapd/inhibit"
+
+static const char *sc_inhibit_dir = SC_INHIBIT_DIR;
+
+bool sc_snap_is_inhibited(const char *snap_name, sc_snap_inhibition_hint hint)
+{
+	char file_name[PATH_MAX] = { 0 };
+	switch (hint) {
+	case SC_SNAP_HINT_INHIBITED_FOR_REMOVE:
+		sc_must_snprintf(file_name, sizeof file_name, "%s.remove",
+				 snap_name);
+		break;
+	default:
+		die("unknown inhibition hint %d", hint);
+	}
+
+	int dirfd SC_CLEANUP(sc_cleanup_close) = -1;
+	dirfd =
+	    open(sc_inhibit_dir, O_CLOEXEC | O_DIRECTORY | O_NOFOLLOW | O_PATH);
+	if (dirfd < 0 && errno == ENOENT) {
+		return false;
+	}
+	if (dirfd < 0) {
+		die("cannot open path %s", sc_inhibit_dir);
+	}
+
+	struct stat file_info;
+	if (fstatat(dirfd, file_name, &file_info, AT_SYMLINK_NOFOLLOW) < 0) {
+		if (errno == ENOENT) {
+			return false;
+		}
+		die("cannot inspect file %s/%s", sc_inhibit_dir, file_name);
+	}
+
+	return S_ISREG(file_info.st_mode);
+}

cmd/libsnap-confine-private/locking.h

@@ -25,6 +25,7 @@
 #endif
 
 #include <sys/types.h>
+#include <stdbool.h>
 
 /**
  * Obtain a flock-based, exclusive, globally scoped, lock.
@@ -104,4 +105,14 @@ void sc_enable_sanity_timeout(void);
  **/
 void sc_disable_sanity_timeout(void);
 
+typedef enum sc_snap_inhibition_hint {
+	SC_SNAP_HINT_INHIBITED_FOR_REMOVE = 1 << 0,
+} sc_snap_inhibition_hint;
+
+/**
+ * sc_snap_is_inhibited returns true if a given inhibition hint is set for given snap.
+ * This is determined by testing the presence of a file in /var/lib/snapd/inhibit/<snap_name>.<hint>.
+**/
+bool sc_snap_is_inhibited(const char *snap_name, sc_snap_inhibition_hint hint);
+
 #endif				// SNAP_CONFINE_LOCKING_H

cmd/snap-confine/snap-confine.c

@@ -732,6 +732,20 @@ static void enter_non_classic_execution_environment(sc_invocation *inv,
 
 	// Do per-snap initialization.
 	int snap_lock_fd = sc_lock_snap(inv->snap_instance);
+
+	// This is a workaround for systemd v237 (used by Ubuntu 18.04) for non-root users
+	// where a transient scope cgroup is not created for a snap hence it cannot be tracked
+	// by the usual snap.<security-tag>-<uuid>.scope pattern.
+	// TODO: Add spread test
+	if (sc_snap_is_inhibited
+	    (inv->snap_instance, SC_SNAP_HINT_INHIBITED_FOR_REMOVE)) {
+		// Prevent starting new snap processes when snap is being removed until
+		// the freezer cgroup is created below and the snap lock is released so
+		// that remove change can track running processes through pids under the
+		// freezer cgroup.
+		die("snap is currently being removed");
+	}
+
 	debug("initializing mount namespace: %s", inv->snap_instance);
 	struct sc_mount_ns *group = NULL;
 	group = sc_open_mount_ns(inv->snap_instance);

overlord/snapstate/backend_test.go

@@ -33,6 +33,7 @@ import (
 	. "gopkg.in/check.v1"
 
 	"github.com/snapcore/snapd/boot"
+	"github.com/snapcore/snapd/cmd/snaplock"
 	"github.com/snapcore/snapd/cmd/snaplock/runinhibit"
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/osutil"
@@ -98,6 +99,8 @@ type fakeOp struct {
 
 	containerName     string
 	containerFileName string
+
+	snapLocked bool
 }
 
 type fakeOps []fakeOp
@@ -1328,9 +1331,17 @@ func (f *fakeSnappyBackend) StopServices(svcs []*snap.AppInfo, reason snap.Servi
 }
 
 func (f *fakeSnappyBackend) KillSnapApps(snapName string, reason snap.AppKillReason, tm timings.Measurer) error {
+	testLock, err := snaplock.OpenLock(snapName)
+	if err != nil {
+		return err
+	}
+	defer testLock.Close()
+
 	f.appendOp(&fakeOp{
 		op:   fmt.Sprintf("kill-snap-apps:%s", reason),
 		name: snapName,
+		// Check if snap lock is held when terminating pids
+		snapLocked: testLock.TryLock() == osutil.ErrAlreadyLocked,
 	})
 	return f.maybeErrForLastOp()
 }

overlord/snapstate/handlers.go

@@ -38,6 +38,7 @@ import (
 	"github.com/snapcore/snapd/boot"
 	"github.com/snapcore/snapd/client"
 	"github.com/snapcore/snapd/client/clientutil"
+	"github.com/snapcore/snapd/cmd/snaplock"
 	"github.com/snapcore/snapd/cmd/snaplock/runinhibit"
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/features"
@@ -3365,6 +3366,21 @@ func (m *SnapManager) doKillSnapApps(t *state.Task, _ *tomb.Tomb) (err error) {
 	}
 	snapName := snapsup.InstanceName()
 
+	// This snap lock syncs snap-confine and this task to make sure they are not racing
+	// on two important resources:
+	//   - Remove inhibition lock (which snap-confine exits when observing)
+	//   - V1 freezer cgroup (which snap-confine creates and joins)
+	// This is needed to address an issue in systemd v237 (used by Ubuntu 18.04) for
+	// non-root users where no tracking transient scope cgroups are created except
+	// the freezer cgroup which is created in snap-confine after the inhibition lock
+	// is release by "snap run".
+	lock, err := snaplock.OpenLock(snapName)
+	if err != nil {
+		return err
+	}
+	lock.Lock()
+	defer lock.Unlock()
+
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snapsup.Revision()}
 	if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedForRemove, inhibitInfo); err != nil {
 		return err

overlord/snapstate/handlers_link_test.go

@@ -34,6 +34,7 @@ import (
 	"github.com/snapcore/snapd/boot/boottest"
 	"github.com/snapcore/snapd/bootloader"
 	"github.com/snapcore/snapd/bootloader/bootloadertest"
+	"github.com/snapcore/snapd/cmd/snaplock"
 	"github.com/snapcore/snapd/cmd/snaplock/runinhibit"
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/osutil"
@@ -2667,8 +2668,9 @@ func (s *linkSnapSuite) testDoKillSnapApps(c *C, svc bool) {
 
 	expected := fakeOps{
 		{
-			op:   "kill-snap-apps:remove",
-			name: "some-snap",
+			op:         "kill-snap-apps:remove",
+			name:       "some-snap",
+			snapLocked: true,
 		},
 	}
 	if svc {
@@ -2684,6 +2686,12 @@ func (s *linkSnapSuite) testDoKillSnapApps(c *C, svc bool) {
 	hint, _, err := runinhibit.IsLocked("some-snap")
 	c.Assert(err, IsNil)
 	c.Check(hint, Equals, runinhibit.HintInhibitedForRemove)
+
+	// Snap lock is unlocked after kill-snap-apps returns
+	testLock, err := snaplock.OpenLock("some-snap")
+	c.Assert(err, IsNil)
+	c.Check(testLock.TryLock(), IsNil)
+	testLock.Close()
 }
 
 func (s *linkSnapSuite) TestDoKillSnapApps(c *C) {
@@ -2736,6 +2744,11 @@ func (s *linkSnapSuite) TestDoKillSnapAppsUnlocksOnError(c *C) {
 	c.Assert(err, IsNil)
 	// On error hint inhibition file is unlocked
 	c.Check(hint, Equals, runinhibit.HintNotInhibited)
+	// And snap lock is also unlocked
+	testLock, err := snaplock.OpenLock("some-snap")
+	c.Assert(err, IsNil)
+	c.Check(testLock.TryLock(), IsNil)
+	testLock.Close()
 }
 
 func (s *linkSnapSuite) testDoUndoKillSnapApps(c *C, svc bool) {
@@ -2787,8 +2800,9 @@ func (s *linkSnapSuite) testDoUndoKillSnapApps(c *C, svc bool) {
 
 	expected := fakeOps{
 		{
-			op:   "kill-snap-apps:remove",
-			name: "some-snap",
+			op:         "kill-snap-apps:remove",
+			name:       "some-snap",
+			snapLocked: true,
 		},
 	}
 	if svc {
@@ -2805,6 +2819,11 @@ func (s *linkSnapSuite) testDoUndoKillSnapApps(c *C, svc bool) {
 	c.Assert(err, IsNil)
 	// On undo hint inhibition file is unlocked
 	c.Check(hint, Equals, runinhibit.HintNotInhibited)
+	// And snap lock is also unlocked
+	testLock, err := snaplock.OpenLock("some-snap")
+	c.Assert(err, IsNil)
+	c.Check(testLock.TryLock(), IsNil)
+	testLock.Close()
 }
 
 func (s *linkSnapSuite) TestDoUndoKillSnapApps(c *C) {

sandbox/cgroup/export_test.go

@@ -147,7 +147,7 @@ func MockThawSnapProcessesImplV1(fn func(snapName string) error) (restore func()
 	return testutil.Mock(&thawSnapProcessesImplV1, fn)
 }
 
-func MockKillProcessesInCgroup(fn func(ctx context.Context, dir string) error) (restore func()) {
+func MockKillProcessesInCgroup(fn func(ctx context.Context, dir string, freeze func(ctx context.Context), thaw func()) error) (restore func()) {
 	return testutil.Mock(&killProcessesInCgroup, fn)
 }
 
@@ -162,3 +162,7 @@ func MockOsReadFile(fn func(name string) ([]byte, error)) (restore func()) {
 func MockMaxKillTimeout(t time.Duration) (restore func()) {
 	return testutil.Mock(&maxKillTimeout, t)
 }
+
+func MockKillThawCooldown(t time.Duration) (restore func()) {
+	return testutil.Mock(&killThawCooldown, t)
+}