Conversation
Coordinate concurrent invocations on incoming_dir via flock() instead of each independently mount/unmounting, which could remove the wrong process's bind-mount layer. First worker in mounts, last one out unmounts.
Contributor
Author
|
CI failed due to three runners getting StoragePoolExceptions |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #675 +/- ##
=======================================
Coverage 71.71% 71.71%
=======================================
Files 3 3
Lines 502 502
=======================================
Hits 360 360
Misses 142 142 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes QubesOS/qubes-issues/issues/5950
Problem
When two
qvm-copyoperations to the same target VM run concurrently,each invocation of
qfile-unpackerindependently bind-mounts and laterunmounts
incoming_dir. Since both mounts stack at the same path inthe same mount namespace,
umount2()can remove the wrong process's mount layer, causing:qfile-unpacker: Fatal error: Cannot umount incoming directory (error type: Invalid argument)Fix
Instead of every invocation independently mounting and unmounting,
coordinate via flock() on incoming_dir itself :
Each worker holds a LOCK_SH on incoming_dir for its entire lifetime.
On entry: attempt LOCK_EX (non-blocking). Winner checks whether
already mounted, mounts if not, downgrades to LOCK_SH. Loser blocks
on LOCK_SH (waiting for the current holder's mount decision), then
re-checks mount state, if still unmounted then releases SH and retries EX.
On exit: release LOCK_SH, blocking-acquire LOCK_EX, check
mount state, unmount only if still mounted.
Crash safety comes from flock() itself since kernel cleans up after a process dies.
mount()/umount2() use the resolved absolute path throughout, since a process's "." can end up resolving through an lazily-detached mount reference.
Testing
Validated qfile-unpacker.c linked
against minimal local stubs of gui-fatal.h/libqubes-rpc-filecopy.h
(do_unpack_ext replaced with a sleep to simulate copy duration, no
change to any mount/lock logic under test).
Tested with N=64 concurrent qfile-unpacker, invocations and numerous iterations with varied timings:
iterations show >1 mount/unmount cycle per batch (late joiner races a
teardown) expected self-healing.
recovers correctly every time.
Important Notes
The mount-status check uses statx(STATX_MNT_ID), which requires kernel
5.8+. I haven't been able to confirm Qubes' actual minimum supported
kernel floor for VMs/templates.
AI disclosure
Used Claude among some other models to discuss, research and write tests against my proposed fixes and review the final file before the files were reviewed and verified by me.