many: add state unlocker to runinhibit helpers #14671

ZeyadYasser · 2024-10-24T19:41:22Z

This avoids the deadlock scenario:

snap run holds inhibition lock.
snapd holds state lock (link-snap).
snapd tries to hold inhibition lock but waits for snap run to release lock.
snap run calls the REST API (i.e. for notices, connections) which tries to hold state lock.
snap run is now waiting for snapd to release state lock (to send the notice) and snapd is waiting for snap run to release the inibition lock.
we have a deadlock and snapd state is locked.

Unlocking snapd state using the new explicitly passed unlocker to runinhibit helpers avoids this deadlock.

Fixes: https://bugs.launchpad.net/snapd/+bug/2084730

This is an alternative approach to #14669

Signed-off-by: Zeyad Gouda <[email protected]>

codecov · 2024-10-24T19:58:32Z

Codecov Report

Attention: Patch coverage is 86.58537% with 11 lines in your changes missing coverage. Please review.

Project coverage is 78.98%. Comparing base (96ea7b0) to head (f162216).
Report is 48 commits behind head on master.

Files with missing lines	Patch %	Lines
overlord/hookstate/hooks.go	50.00%	0 Missing and 5 partials ⚠️
overlord/snapstate/handlers.go	81.81%	2 Missing and 2 partials ⚠️
overlord/snapstate/backend/link.go	91.66%	0 Missing and 1 partial ⚠️
overlord/snapstate/backend/locking.go	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14671      +/-   ##
==========================================
+ Coverage   78.95%   78.98%   +0.03%     
==========================================
  Files        1084     1085       +1     
  Lines      146638   147222     +584     
==========================================
+ Hits       115773   116280     +507     
- Misses      23667    23717      +50     
- Partials     7198     7225      +27

Flag	Coverage Δ
unittests	`78.98% <86.58%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bboozzoo

Looks quite simple.

Can we add a spread test where the hint file is held locked by the test?

bboozzoo · 2024-10-25T12:23:50Z

cmd/snaplock/runinhibit/inhibit.go

@@ -132,7 +138,13 @@ 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 {
+func LockWithHint(snapName string, hint Hint, info InhibitInfo, unlocker Unlocker) error {


doc comments need to be updated to mention unlocker

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser · 2024-10-28T15:27:09Z

Can we add a spread test where the hint file is held locked by the test?

Added, Thanks for pointing it out!

MiguelPires

looks good. It's a bit of a leaky abstraction but for now it does seem better than doing it manually in snapstate/ which is likely more error-prone

bboozzoo · 2024-10-29T21:35:21Z

tests/regression/lp-2084730/task.yaml

+    # This refresh will block because it tries to hold the inhibition file lock for test-snapd-sh
+    snap refresh --no-wait --edge test-snapd-sh > locked-change-id
+
+    # Check that snapd state is not locked when trying to hold the inhibition file lock above


prob this should do

# wait until snapd is blocked in link-snap # avoid using the API directly to not take the state lock locked_id="$(cat locked-change-id)" retry -n 50 --wait 1 sh -c "snap debug state /var/lib/snapd/state.json $locked_id | MATCH 'Doing .* Make snap .* available to the system'" # and still waiting snap debug state /var/lib/snapd/state.json $locked_id | MATCH 'Doing .* Make snap .* available to the system'

not sure, but maybe it would be worth to confirm with lsof that snapd is holding the lock file open

thanks, also added the lsof check

bboozzoo · 2024-10-29T21:36:18Z

tests/regression/lp-2084730/task.yaml

+
+    # Wait for refresh to finish to be able to remove the snap
+    snap debug ensure-state-soon
+    retry -n 10 sh -c "snap changes | NOMATCH Doing"


I think this should work:

if [ -s locked-change-id ]; then snap wait $(cat locked-change-id) fi

bboozzoo · 2024-10-29T21:36:31Z

tests/regression/lp-2084730/task.yaml

+    snap debug ensure-state-soon
+    retry -n 10 sh -c "snap changes | NOMATCH Doing"
+
+    snap remove --purge test-snapd-sh


cleanup does it already

bboozzoo · 2024-10-29T22:06:48Z

tests/regression/lp-2084730/task.yaml

+    # This refresh will block because it tries to hold the inhibition file lock for test-snapd-sh
+    snap refresh --no-wait --edge test-snapd-sh > locked-change-id
+
+    # Check that snapd state is not locked when trying to hold the inhibition file lock above


Can you check that when a snapd is stuck on flock, operations changing the state of the snap (remove, or another refresh) fail the conflict check?

good point, added those checks. thank you!

also, added checks installing a parallel instance

Meulengracht

Thank you, FWIW I think the fix is a bit too elaborate but I think given how Inhibitors are done today its fine. The test comments from maciej would be great to have resolved

Signed-off-by: Zeyad Gouda <[email protected]>

bboozzoo

Thanks. It'd be good to ask @pedronis to have a look as well.

pedronis

some questions

overlord/hookstate/ctlcmd/refresh.go

pedronis · 2024-11-05T12:59:23Z

overlord/hookstate/hooks.go

@@ -177,7 +177,7 @@ func (h *gateAutoRefreshHookHandler) Before() error {
 	defer lock.Unlock()


same question about the snap lock

pedronis · 2024-11-05T13:04:01Z

overlord/hookstate/hooks.go

+			_, inhibitInfo, err := runinhibit.IsLocked(snapName, st.Unlocker())
 			if err != nil {
 				return err
 			}
-			if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedForRefresh, inhibitInfo); err != nil {
+			if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedForRefresh, inhibitInfo, st.Unlocker()); err != nil {


this is going to unlock/lock the state twice quickly, it feels like it would be better later to add a single operation that does this

updated, thanks for catching this

overlord/snapstate/handlers.go

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]>

Signed-off-by: Zeyad Gouda <[email protected]>

pedronis

thank you

ZeyadYasser · 2024-11-11T15:35:57Z

Following failing tests are known to fail:

google-distro-2:opensuse-tumbleweed-64:tests/main/document-portal-activation
google-distro-2:opensuse-tumbleweed-64:tests/main/prepare-image-validation-sets
google-distro-2:opensuse-tumbleweed-64:tests/main/sudo-env
google:ubuntu-18.04-64:tests/main/snap-seccomp-syscalls
google:ubuntu-24.10-64:tests/main/apparmor-prompting-integration-tests:write_single_deny
google-pro:ubuntu-fips-22.04-64:tests/fips/

ZeyadYasser added 4 commits October 24, 2024 21:12

many: add state unlocker to runinhibit.LockWithHint

d209d73

Signed-off-by: Zeyad Gouda <[email protected]>

many: add state unlocker to runinhibit.Unlock

ce5f75e

Signed-off-by: Zeyad Gouda <[email protected]>

many: add state unlocker to runinhibit.IsLocked

9afda6f

Signed-off-by: Zeyad Gouda <[email protected]>

many: add state unlocker to runinhibit.RemoveLockFile

1226e84

Signed-off-by: Zeyad Gouda <[email protected]>

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Oct 24, 2024

ZeyadYasser requested review from bboozzoo, pedronis and Meulengracht October 24, 2024 19:41

ZeyadYasser added the Needs Samuele review Needs a review from Samuele before it can land label Oct 24, 2024

bboozzoo reviewed Oct 25, 2024

View reviewed changes

ZeyadYasser added this to the 2.67 milestone Oct 28, 2024

ZeyadYasser added 2 commits October 28, 2024 16:22

cmd/snaplock/runinhibit: update doc comment to mention unlocker

1bc55ec

Signed-off-by: Zeyad Gouda <[email protected]>

tests: add a regression spread test for lp-2084730

d501783

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from bboozzoo October 28, 2024 15:27

MiguelPires approved these changes Oct 29, 2024

View reviewed changes

bboozzoo reviewed Oct 29, 2024

View reviewed changes

Meulengracht approved these changes Oct 30, 2024

View reviewed changes

ZeyadYasser added 2 commits October 30, 2024 15:19

o/snapstate/backend: add unit tests for forbidden nil state unlocker

ab2f480

Signed-off-by: Zeyad Gouda <[email protected]>

tests: address review comments

d639545

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from bboozzoo October 30, 2024 14:32

ZeyadYasser added 2 commits October 31, 2024 10:30

tests: fix restore for tests/regression/lp-2084730

70100ec

Signed-off-by: Zeyad Gouda <[email protected]>

tests: skip lsof command if not found

5112953

Signed-off-by: Zeyad Gouda <[email protected]>

bboozzoo approved these changes Nov 1, 2024

View reviewed changes

pedronis reviewed Nov 5, 2024

View reviewed changes

ZeyadYasser added 2 commits November 11, 2024 10:04

o/hookstate: avoid unlocking/locking state twice quickly

f162216

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from pedronis November 11, 2024 08:42

pedronis approved these changes Nov 11, 2024

View reviewed changes

ZeyadYasser requested a review from MiguelPires November 11, 2024 10:07

MiguelPires approved these changes Nov 11, 2024

View reviewed changes

Meulengracht merged commit 2905448 into canonical:master Nov 12, 2024
54 of 58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: add state unlocker to runinhibit helpers #14671

many: add state unlocker to runinhibit helpers #14671

ZeyadYasser commented Oct 24, 2024

codecov bot commented Oct 24, 2024 •

edited

Loading

bboozzoo left a comment

bboozzoo Oct 25, 2024

ZeyadYasser Oct 28, 2024

ZeyadYasser commented Oct 28, 2024

MiguelPires left a comment •

edited

Loading

bboozzoo Oct 29, 2024

ZeyadYasser Oct 30, 2024

bboozzoo Oct 29, 2024

ZeyadYasser Oct 30, 2024

bboozzoo Oct 29, 2024

bboozzoo Oct 29, 2024

ZeyadYasser Oct 30, 2024

ZeyadYasser Oct 30, 2024

Meulengracht left a comment •

edited

Loading

bboozzoo left a comment

pedronis left a comment

pedronis Nov 5, 2024

pedronis Nov 5, 2024

ZeyadYasser Nov 11, 2024

pedronis left a comment

ZeyadYasser commented Nov 11, 2024

		@@ -177,7 +177,7 @@ func (h *gateAutoRefreshHookHandler) Before() error {
		defer lock.Unlock()

many: add state unlocker to runinhibit helpers #14671

many: add state unlocker to runinhibit helpers #14671

Conversation

ZeyadYasser commented Oct 24, 2024

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser commented Oct 28, 2024

MiguelPires left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meulengracht left a comment • edited Loading

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

ZeyadYasser commented Nov 11, 2024

codecov bot commented Oct 24, 2024 •

edited

Loading

MiguelPires left a comment •

edited

Loading

Meulengracht left a comment •

edited

Loading