From cee8201989f3c42bdb004c77198b650a681da433 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 20 Nov 2020 16:48:28 -0500 Subject: [PATCH 1/8] Always snapshot in the FILE pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We must snapshot a VM’s disk before exporting it. Otherwise, we will likely corrupt the VM’s filesystem. Fixes https://github.com/QubesOS/qubes-issues/issues/4324 --- qubes/storage/file.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 2c5e9e22..93128bb2 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -265,9 +265,7 @@ class FileVolume(qubes.storage.Volume): return self def export(self): - # FIXME: this should rather return snapshot(self.path, self.path_cow) - # if domain is running - return self.path + return snapshot(self.path, self.path_cow) @asyncio.coroutine def import_volume(self, src_volume): From 14e9154e4e283187f893939eea72b294f07ce178 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 24 Nov 2020 19:15:25 -0500 Subject: [PATCH 2/8] file pool: snapshotting dirty volume not supported Raise a NotImplementedError rather than risking corruption. --- qubes/storage/file.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 93128bb2..22c2d8a4 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -265,7 +265,9 @@ class FileVolume(qubes.storage.Volume): return self def export(self): - return snapshot(self.path, self.path_cow) + if self.is_dirty(): + self._not_implemented('exporting a dirty volume') + return self.path @asyncio.coroutine def import_volume(self, src_volume): From e4854df42f97e9fcb61664059fdfac520b19a3a5 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 25 Nov 2020 13:25:57 -0500 Subject: [PATCH 3/8] File volumes are started NAND exported So add a lock to ensure this. --- qubes/storage/file.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 22c2d8a4..e5dadb18 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -176,11 +176,14 @@ class FilePool(qubes.storage.Pool): class FileVolume(qubes.storage.Volume): ''' Parent class for the xen volumes implementation which expects a `target_dir` param on initialization. ''' + _marker_running = object() + _marker_exported = object() def __init__(self, dir_path, **kwargs): self.dir_path = dir_path assert self.dir_path, "dir_path not specified" self._revisions_to_keep = 0 + self._locked = None super().__init__(**kwargs) if self.snap_on_start: @@ -265,10 +268,18 @@ class FileVolume(qubes.storage.Volume): return self def export(self): - if self.is_dirty(): + if self._locked is not None: + assert self._locked is FileVolume._marker_running, \ + 'nested calls to export()' self._not_implemented('exporting a dirty volume') + self._locked = FileVolume._marker_exported return self.path + def export_end(self, path): + assert self._locked is FileVolume._marker_exported, \ + 'cannot end an export that never began' + self._locked = None + @asyncio.coroutine def import_volume(self, src_volume): if src_volume.snap_on_start: @@ -311,6 +322,11 @@ class FileVolume(qubes.storage.Volume): return self def start(self): + if self._locked is not None: + assert self._locked is FileVolume._marker_exported, \ + 'nested calls to start()' + self._not_implemented('starting a VM with an exported volume') + self._locked = FileVolume._marker_running if not self.save_on_stop and not self.snap_on_start: self.reset() else: @@ -328,12 +344,15 @@ class FileVolume(qubes.storage.Volume): return self def stop(self): + assert self._locked is FileVolume._marker_running, \ + 'cannot stop a volume that has not been started' if self.save_on_stop: self.commit() elif self.snap_on_start: _remove_if_exists(self.path_cow) else: _remove_if_exists(self.path) + self._locked = None return self @property From e53d04005120b472b820b8732bf8d96ae495a5df Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 25 Nov 2020 13:33:32 -0500 Subject: [PATCH 4/8] Re-add dirty check in case qubesd is restarted --- qubes/storage/file.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index e5dadb18..102bbafd 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -326,6 +326,8 @@ class FileVolume(qubes.storage.Volume): assert self._locked is FileVolume._marker_exported, \ 'nested calls to start()' self._not_implemented('starting a VM with an exported volume') + if self.is_dirty(): + self._not_implemented('exporting a dirty volume') self._locked = FileVolume._marker_running if not self.save_on_stop and not self.snap_on_start: self.reset() From ec51673f21230a09b4847afefc350c200fad5314 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 25 Nov 2020 15:08:13 -0500 Subject: [PATCH 5/8] Fix export locking If qubesd has restarted then _export_lock will be None --- qubes/storage/file.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 102bbafd..7a89d8e1 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -183,7 +183,7 @@ class FileVolume(qubes.storage.Volume): self.dir_path = dir_path assert self.dir_path, "dir_path not specified" self._revisions_to_keep = 0 - self._locked = None + self._export_lock = None super().__init__(**kwargs) if self.snap_on_start: @@ -268,17 +268,17 @@ class FileVolume(qubes.storage.Volume): return self def export(self): - if self._locked is not None: - assert self._locked is FileVolume._marker_running, \ + if self._export_lock is not None: + assert self._export_lock is FileVolume._marker_running, \ 'nested calls to export()' self._not_implemented('exporting a dirty volume') - self._locked = FileVolume._marker_exported + self._export_lock = FileVolume._marker_exported return self.path def export_end(self, path): - assert self._locked is FileVolume._marker_exported, \ - 'cannot end an export that never began' - self._locked = None + assert self._export_lock is not FileVolume._marker_running, \ + 'ending an export on a running volume?' + self._export_lock = None @asyncio.coroutine def import_volume(self, src_volume): @@ -322,13 +322,13 @@ class FileVolume(qubes.storage.Volume): return self def start(self): - if self._locked is not None: - assert self._locked is FileVolume._marker_exported, \ + if self._export_lock is not None: + assert self._export_lock is FileVolume._marker_exported, \ 'nested calls to start()' self._not_implemented('starting a VM with an exported volume') if self.is_dirty(): self._not_implemented('exporting a dirty volume') - self._locked = FileVolume._marker_running + self._export_lock = FileVolume._marker_running if not self.save_on_stop and not self.snap_on_start: self.reset() else: @@ -346,15 +346,15 @@ class FileVolume(qubes.storage.Volume): return self def stop(self): - assert self._locked is FileVolume._marker_running, \ - 'cannot stop a volume that has not been started' + assert self._export_lock is not FileVolume._marker_exported, \ + 'trying to stop exported file volume?' if self.save_on_stop: self.commit() elif self.snap_on_start: _remove_if_exists(self.path_cow) else: _remove_if_exists(self.path) - self._locked = None + self._export_lock = None return self @property From 7275939000799ce9568e694c5ee7995ccb774f1e Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 25 Nov 2020 16:42:35 -0500 Subject: [PATCH 6/8] Fix bugs found by Rusty Bird --- qubes/storage/file.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 7a89d8e1..00293494 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -271,6 +271,8 @@ class FileVolume(qubes.storage.Volume): if self._export_lock is not None: assert self._export_lock is FileVolume._marker_running, \ 'nested calls to export()' + self._not_implemented('exporting a starting volume') + if self.is_dirty(): self._not_implemented('exporting a dirty volume') self._export_lock = FileVolume._marker_exported return self.path @@ -326,8 +328,6 @@ class FileVolume(qubes.storage.Volume): assert self._export_lock is FileVolume._marker_exported, \ 'nested calls to start()' self._not_implemented('starting a VM with an exported volume') - if self.is_dirty(): - self._not_implemented('exporting a dirty volume') self._export_lock = FileVolume._marker_running if not self.save_on_stop and not self.snap_on_start: self.reset() From 09785449edd55b151bfca76b074944944be3760d Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 26 Nov 2020 11:17:33 -0500 Subject: [PATCH 7/8] Return better error messages from file pool A `qubes.storage.StoragePoolException` will be returned as a useful error from `qvm-backup`. --- qubes/storage/file.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 00293494..83bd0ce3 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -271,9 +271,9 @@ class FileVolume(qubes.storage.Volume): if self._export_lock is not None: assert self._export_lock is FileVolume._marker_running, \ 'nested calls to export()' - self._not_implemented('exporting a starting volume') + raise qubes.storage.StoragePoolException('file pool cannot export running volumes') if self.is_dirty(): - self._not_implemented('exporting a dirty volume') + raise qubes.storage.StoragePoolException('file pool cannot export dirty volumes') self._export_lock = FileVolume._marker_exported return self.path @@ -327,7 +327,7 @@ class FileVolume(qubes.storage.Volume): if self._export_lock is not None: assert self._export_lock is FileVolume._marker_exported, \ 'nested calls to start()' - self._not_implemented('starting a VM with an exported volume') + raise qubes.storage.StoragePoolException('file pool cannot start a VM with an exported volume') self._export_lock = FileVolume._marker_running if not self.save_on_stop and not self.snap_on_start: self.reset() From 542fee173dc59a56958764007f73a66281e8df01 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 26 Nov 2020 13:40:07 -0500 Subject: [PATCH 8/8] Fix line lengths --- qubes/storage/file.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 83bd0ce3..c2374272 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -271,9 +271,11 @@ class FileVolume(qubes.storage.Volume): if self._export_lock is not None: assert self._export_lock is FileVolume._marker_running, \ 'nested calls to export()' - raise qubes.storage.StoragePoolException('file pool cannot export running volumes') + raise qubes.storage.StoragePoolException( + 'file pool cannot export running volumes') if self.is_dirty(): - raise qubes.storage.StoragePoolException('file pool cannot export dirty volumes') + raise qubes.storage.StoragePoolException( + 'file pool cannot export dirty volumes') self._export_lock = FileVolume._marker_exported return self.path @@ -327,7 +329,8 @@ class FileVolume(qubes.storage.Volume): if self._export_lock is not None: assert self._export_lock is FileVolume._marker_exported, \ 'nested calls to start()' - raise qubes.storage.StoragePoolException('file pool cannot start a VM with an exported volume') + raise qubes.storage.StoragePoolException( + 'file pool cannot start a VM with an exported volume') self._export_lock = FileVolume._marker_running if not self.save_on_stop and not self.snap_on_start: self.reset()