many: add state unlocker to runinhibit helpers (canonical#14671)

* many: add state unlocker to runinhibit.LockWithHint Signed-off-by: Zeyad Gouda <[email protected]> * many: add state unlocker to runinhibit.Unlock Signed-off-by: Zeyad Gouda <[email protected]> * many: add state unlocker to runinhibit.IsLocked Signed-off-by: Zeyad Gouda <[email protected]> * many: add state unlocker to runinhibit.RemoveLockFile Signed-off-by: Zeyad Gouda <[email protected]> * cmd/snaplock/runinhibit: update doc comment to mention unlocker Signed-off-by: Zeyad Gouda <[email protected]> * tests: add a regression spread test for lp-2084730 Signed-off-by: Zeyad Gouda <[email protected]> * o/snapstate/backend: add unit tests for forbidden nil state unlocker Signed-off-by: Zeyad Gouda <[email protected]> * tests: address review comments Signed-off-by: Zeyad Gouda <[email protected]> * tests: fix restore for tests/regression/lp-2084730 Signed-off-by: Zeyad Gouda <[email protected]> * tests: skip lsof command if not found Signed-off-by: Zeyad Gouda <[email protected]> * cmd/snaplock: add comment about potential deadlock using snap lock The snap lock is only accessible to root and is only intended to synchronize operations between snapd and snap-confine (and snap-update-ns in some cases). Any process holding the snap lock must not do any interactions with snapd to avoid deadlocks due to locked snap state. Signed-off-by: Zeyad Gouda <[email protected]> * o/hookstate: avoid unlocking/locking state twice quickly Signed-off-by: Zeyad Gouda <[email protected]> --------- Signed-off-by: Zeyad Gouda <[email protected]>
pedronis · Nov 12, 2024 · 2905448 · 2905448
1 parent f17d3ac
commit 2905448
Show file tree

Hide file tree

Showing 23 changed files with 412 additions and 127 deletions.
diff --git a/cmd/snap/cmd_run.go b/cmd/snap/cmd_run.go
@@ -501,7 +501,7 @@ func (x *cmdRun) straceOpts() (opts []string, raw bool, err error) {
 func checkSnapRunInhibitionConflict(app *snap.AppInfo) error {
 	// Remove hint check takes precedence because we want to exit early
 	snapName := app.Snap.InstanceName()
-	hint, _, err := runinhibit.IsLocked(snapName)
+	hint, _, err := runinhibit.IsLocked(snapName, nil)
 	if err != nil {
 		return err
 	}

diff --git a/cmd/snap/cmd_run_test.go b/cmd/snap/cmd_run_test.go
@@ -281,7 +281,7 @@ func (s *RunSuite) TestSnapRunAppRunsChecksRefreshInhibitionLock(c *check.C) {
 	defer restorer()
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R("x2")}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil)
 	c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil)
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
 
@@ -321,7 +321,7 @@ func (s *RunSuite) TestSnapRunAppRunsChecksRefreshInhibitionLock(c *check.C) {
 
 func (s *RunSuite) testSnapRunAppRunsChecksRemoveInhibitionLock(c *check.C, svc bool) {
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, inhibitInfo), check.IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, inhibitInfo, nil), check.IsNil)
 
 	cmd := "snapname.app"
 	if svc {
@@ -355,7 +355,7 @@ func (s *RunSuite) TestSnapRunAppRefreshAppAwarenessUnsetSkipsInhibitionLockWait
 
 	// mark snap as inhibited
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R("x2")}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil)
 	c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil)
 	// unset refresh-app-awareness flag
 	c.Assert(os.RemoveAll(features.RefreshAppAwareness.ControlFile()), check.IsNil)
@@ -379,7 +379,7 @@ func (s *RunSuite) TestSnapRunAppNewRevisionAfterInhibition(c *check.C) {
 
 	// mark snap as inhibited
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R("x2")}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil)
 	// unset refresh-app-awareness flag
 	c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil)
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
@@ -498,7 +498,7 @@ func (s *RunSuite) TestSnapRunHookNoRuninhibit(c *check.C) {
 	defer restore()
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(42)}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil)
 	c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil)
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
 
@@ -532,7 +532,7 @@ func (s *RunSuite) TestSnapRunAppRuninhibitSkipsServices(c *check.C) {
 	defer restorer()
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R("x2")}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil)
 	c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil)
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
 
@@ -718,7 +718,7 @@ func (s *RunSuite) testSnapRunAppRetryNoInhibitHintFileThenOngoingRefresh(c *che
 			// mock snap inhibited to trigger race condition detection
 			// i.e. we started without a hint lock file (snap on first install)
 			// then a refresh started which created the hint lock file.
-			c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}), check.IsNil)
+			c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}, nil), check.IsNil)
 
 			// nil FileLock means no inhibit file exists
 			return nil, nil
@@ -847,7 +847,7 @@ func (s *RunSuite) testSnapRunAppRetryNoInhibitHintFileThenOngoingRemove(c *chec
 		// mock snap inhibited to trigger race condition detection
 		// i.e. we started without a hint lock file (snap on first install)
 		// then a remove started which created the hint lock file.
-		c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, runinhibit.InhibitInfo{Previous: snap.R("x2")}), check.IsNil)
+		c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, runinhibit.InhibitInfo{Previous: snap.R("x2")}, nil), check.IsNil)
 
 		// nil FileLock means no inhibit file exists
 		return nil, nil
@@ -955,7 +955,7 @@ func (s *RunSuite) TestSnapRunAppRetryNoInhibitHintFileThenOngoingRefreshMissing
 			// and we have an ongoing refresh which removed current symlink.
 			c.Assert(err, testutil.ErrorIs, snaprun.ErrSnapRefreshConflict)
 			// and created the inhibition hint lock file.
-			c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}), check.IsNil)
+			c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}, nil), check.IsNil)
 			return nil, err
 		} else {
 			var err error
@@ -1054,7 +1054,7 @@ func (s *RunSuite) TestSnapRunAppMaxRetry(c *check.C) {
 		// mock snap inhibited to trigger race condition detection
 		// i.e. we started without a hint lock file (snap on first install)
 		// then a refresh started which created the hint lock file.
-		c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}), check.IsNil)
+		c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}, nil), check.IsNil)
 
 		// nil FileLock means no inhibit file exists
 		return nil, nil

diff --git a/cmd/snap/inhibit_test.go b/cmd/snap/inhibit_test.go
@@ -71,7 +71,7 @@ func (s *RunSuite) TestWaitWhileInhibitedRunThrough(c *C) {
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil)
 
 	var waitWhileInhibitedCalled int
 	restore := snaprun.MockWaitWhileInhibited(func(ctx context.Context, snapName string, notInhibited func(ctx context.Context) error, inhibited func(ctx context.Context, hint runinhibit.Hint, inhibitInfo *runinhibit.InhibitInfo) (cont bool, err error), interval time.Duration) (flock *osutil.FileLock, retErr error) {
@@ -130,7 +130,7 @@ func (s *RunSuite) TestWaitWhileInhibitedErrorOnStartNotification(c *C) {
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil)
 
 	var startCalled, finishCalled int
 	inhibitionFlow := fakeInhibitionFlow{
@@ -166,7 +166,7 @@ func (s *RunSuite) TestWaitWhileInhibitedErrorOnFinishNotification(c *C) {
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil)
 
 	var waitWhileInhibitedCalled int
 	restore := snaprun.MockWaitWhileInhibited(func(ctx context.Context, snapName string, notInhibited func(ctx context.Context) error, inhibited func(ctx context.Context, hint runinhibit.Hint, inhibitInfo *runinhibit.InhibitInfo) (cont bool, err error), interval time.Duration) (flock *osutil.FileLock, retErr error) {
@@ -226,7 +226,7 @@ func (s *RunSuite) TestWaitWhileInhibitedContextCancellationOnError(c *C) {
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil)
 
 	originalCtx, cancel := context.WithCancel(context.Background())
 	inhibitionFlow := fakeInhibitionFlow{
@@ -259,7 +259,7 @@ func (s *RunSuite) TestWaitWhileInhibitedGateRefreshNoNotification(c *C) {
 	c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil)
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedGateRefresh, inhibitInfo), IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedGateRefresh, inhibitInfo, nil), IsNil)
 
 	var called int
 	restore := snaprun.MockWaitWhileInhibited(func(ctx context.Context, snapName string, notInhibited func(ctx context.Context) error, inhibited func(ctx context.Context, hint runinhibit.Hint, inhibitInfo *runinhibit.InhibitInfo) (cont bool, err error), interval time.Duration) (flock *osutil.FileLock, retErr error) {
@@ -310,7 +310,7 @@ func (s *RunSuite) testWaitWhileInhibitedRemoveInhibition(c *C, svc bool) {
 	snaptest.MockSnapCurrent(c, string(mockYaml), &snap.SideInfo{Revision: snap.R(11)})
 
 	inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)}
-	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, inhibitInfo), IsNil)
+	c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, inhibitInfo, nil), IsNil)
 
 	inhibitionFlow := fakeInhibitionFlow{
 		start: func(ctx context.Context) error {

diff --git a/cmd/snaplock/lock.go b/cmd/snaplock/lock.go
@@ -36,6 +36,11 @@ func lockFileName(snapName string) string {
 }
 
 // OpenLock creates and opens a lock file associated with a particular snap.
+//
+// NOTE: The snap lock is only accessible to root and is only intended to
+// synchronize operations between snapd and snap-confine (and snap-update-ns
+// in some cases). Any process holding the snap lock must not do any
+// interactions with snapd to avoid deadlocks due to locked snap state.
 func OpenLock(snapName string) (*osutil.FileLock, error) {
 	if err := os.MkdirAll(dirs.SnapRunLockDir, 0700); err != nil {
 		return nil, fmt.Errorf("cannot create lock directory: %s", err)

diff --git a/cmd/snaplock/runinhibit/inhibit.go b/cmd/snaplock/runinhibit/inhibit.go
@@ -124,6 +124,12 @@ func removeInhibitInfoFiles(snapName string) error {
 	return nil
 }
 
+// Unlocker functions are passed from code using runinhibit to indicate that global
+// state should be unlocked during file lock operations to avoid having deadlocks where
+// both inhibition hint file and state are locked and waiting for each other. Unlocker
+// being nil indicates not to do this.
+type Unlocker func() (relock func())
+
 // LockWithHint sets a persistent "snap run" inhibition lock, for the given snap, with a given hint
 // and saves given info that will be needed by "snap run" during inhibition (e.g. snap revision).
 //
@@ -132,7 +138,17 @@ func removeInhibitInfoFiles(snapName string) error {
 // start and will block, presenting a user interface if possible. Also
 // info.Previous corresponding to the snap revision that was installed must be
 // provided and cannot be unset.
-func LockWithHint(snapName string, hint Hint, info InhibitInfo) error {
+//
+// If unlocker is passed it indicates that the global state needs to be unlocked
+// before taking the inhibition hint file lock. It is the responsibility of the
+// caller to make sure state is locked if a non-nil unlocker is passed.
+func LockWithHint(snapName string, hint Hint, info InhibitInfo, unlocker Unlocker) error {
+	if unlocker != nil {
+		// unlock/relock global state
+		relock := unlocker()
+		defer relock()
+	}
+
 	if err := hint.validate(); err != nil {
 		return err
 	}
@@ -179,7 +195,17 @@ func LockWithHint(snapName string, hint Hint, info InhibitInfo) error {
 // Unlock truncates the run inhibition lock, for the given snap.
 //
 // An empty inhibition lock means uninhibited "snap run".
-func Unlock(snapName string) error {
+//
+// If unlocker is passed it indicates that the global state needs to be unlocked
+// before taking the inhibition hint file lock. It is the responsibility of the
+// caller to make sure state is locked if a non-nil unlocker is passed.
+func Unlock(snapName string, unlocker Unlocker) error {
+	if unlocker != nil {
+		// unlock/relock global state
+		relock := unlocker()
+		defer relock()
+	}
+
 	flock, err := openHintFileLock(snapName)
 	if os.IsNotExist(err) {
 		return nil
@@ -214,7 +240,17 @@ func Unlock(snapName string) error {
 //
 // It returns the current, non-empty hint if inhibition is in place. Otherwise
 // it returns an empty hint.
-func IsLocked(snapName string) (Hint, InhibitInfo, error) {
+//
+// If unlocker is passed it indicates that the global state needs to be unlocked
+// before taking the inhibition hint file lock. It is the responsibility of the
+// caller to make sure state is locked if a non-nil unlocker is passed.
+func IsLocked(snapName string, unlocker Unlocker) (Hint, InhibitInfo, error) {
+	if unlocker != nil {
+		// unlock/relock global state
+		relock := unlocker()
+		defer relock()
+	}
+
 	hintFlock, err := osutil.OpenExistingLockForReading(HintFile(snapName))
 	if os.IsNotExist(err) {
 		return "", InhibitInfo{}, nil
@@ -254,7 +290,17 @@ func IsLocked(snapName string) (Hint, InhibitInfo, error) {
 // it.
 //
 // The function does not fail if the inhibition lock does not exist.
-func RemoveLockFile(snapName string) error {
+//
+// If unlocker is passed it indicates that the global state needs to be unlocked
+// before taking the inhibition hint file lock. It is the responsibility of the
+// caller to make sure state is locked if a non-nil unlocker is passed.
+func RemoveLockFile(snapName string, unlocker Unlocker) error {
+	if unlocker != nil {
+		// unlock/relock global state
+		relock := unlocker()
+		defer relock()
+	}
+
 	hintFlock, err := osutil.OpenExistingLockForReading(HintFile(snapName))
 	if os.IsNotExist(err) {
 		return nil