Repository navigation
internal/server/instance/drivers: Unmount residual Unix devices during cleanup - #3979
Merged
Merged
Conversation
stgraber
reviewed
Sep 11, 2026
| "github.com/lxc/incus/v7/shared/logger" | ||
| ) | ||
|
|
||
| func TestRemoveUnixDevicesAfterFailedRestore(t *testing.T) { |
stgraber
reviewed
Sep 11, 2026
| err := os.Remove(devicePath) | ||
| if err != nil { | ||
| d.logger.Error("Failed removing unix device", logger.Ctx{"err": err, "path": devicePath}) | ||
| err := unix.Unmount(devicePath, unix.MNT_DETACH) |
Member
There was a problem hiding this comment.
We should never use detach as that can then hide a mounted filesystem leading to data corruption or system crashes when we then go to the storage layer to delete it.
stgraber
requested changes
Sep 11, 2026
…g cleanup Signed-off-by: Simon Zhou <jp.zdm2008@gmail.com>
fivetime
force-pushed
the
upstream/unix-mount-cleanup-20260911
branch
from
September 14, 2026 01:57
56f03b9 to
f3cff86
Compare
Contributor
Author
|
Changed to a normal unmount (flags 0), retaining the mountpoint and returning the error on Verified both failures in a private mount namespace, including that cleanup succeeds after releasing the busy reference. The previous lazy-detach version incorrectly returned success for that busy mount. The full unit suite and static analysis pass. |
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.
Failed restore cleanup can reach
removeUnixDeviceswith device bind mounts still present. Removing their mountpoints returnsEBUSY, leaving the entries behind.Unmount them normally before removing the entries. Ignore only the expected not-mounted/already-gone errors; collect other unmount/removal errors and return them after attempting the remaining devices. An unmount failure keeps its mountpoint intact. This uses no lazy detach.
A separate private mount-namespace check exercised failed-restore cleanup, repeated cleanup, a real
EPERMfailure after droppingCAP_SYS_ADMIN, and a realEBUSYfailure while the current directory held the bind mount busy. The busy mount and its contents remained accessible; cleanup succeeded after releasing the reference. The previousMNT_DETACHimplementation failed this check by returning success for the busy mount.Validation: the separate runtime check with the race detector,
go test -v ./..., andmake static-analysis. The added test file was removed from this PR following review. The runtime check injects cleanup state directly and does not claim a full CRIU migration test.