Merge remote-tracking branch 'origin/pr/690'

* origin/pr/690:
  use dirty file instead of clean file
  storage: prune reflinks files when snapshots are disabled
  storage: prevent cloning when volumes without snapshot is running
  lint
  don't allow to enable or disable snapshots if dispvm base on current vm is running
  prevent startup of vm having snapshots disabled and running DispVM
  prevent startup if source volume snapshot disabled and is running
  storage: add a running state file for volumes
  storage: add a running state file for volumes
  storage: prevent setting revisions_to_keep < -1
  backup: prevent backup of running VM when private volume snapshots are disabled
  prevent re-enabling snapshots when running VM
  storage: prevent setting disable snapshots when VM is running
  storage: snapshots_disabled for zfs storage
  storage: snapshots_disabled for reflink storage
  storage: snapshots_disabled for lvm storage
  storage: snapshots_disabled for file storage
  storage: add a new volume property snapshots_disabled
  api: update validate_size method to allow negative numbers

Pull request description:

This PR is related to issue QubesOS/qubes-issues#8767

A new option **disable snapshots** can be set on volumes (lvm, file, reflink and zfs supported). When enabled on a private volume, the volume is used directly instead of doing a snapshot and use it.

The option can be enabled by setting ``revisions_to_keep = -1``

Using the volume directly saves space but requires features like backup, dispvm or cloning to be disabled when the volume is in use.

Author: marmarek

qubes/api/__init__.py

@@ -245,15 +245,21 @@ def enforce(predicate):
         if not predicate:
             raise PermissionDenied()
 
-    def validate_size(self, untrusted_size: bytes) -> int:
+    def validate_size(
+        self, untrusted_size: bytes, allow_negative: bool = False
+    ) -> int:
         self.enforce(isinstance(untrusted_size, bytes))
+        coefficient = 1
+        if allow_negative and untrusted_size.startswith(b"-"):
+            coefficient = -1
+            untrusted_size = untrusted_size[1:]
         if not untrusted_size.isdigit():
             raise qubes.exc.ProtocolError("Size must be ASCII digits (only)")
         if len(untrusted_size) >= 20:
             raise qubes.exc.ProtocolError("Sizes limited to 19 decimal digits")
         if untrusted_size[0] == 48 and untrusted_size != b"0":
             raise qubes.exc.ProtocolError("Spurious leading zeros not allowed")
-        return int(untrusted_size)
+        return int(untrusted_size) * coefficient
 
 
 class QubesDaemonProtocol(asyncio.Protocol):

qubes/api/admin.py

@@ -502,7 +502,7 @@ async def vm_volume_clone_to(self, untrusted_payload):
             src_volume=src_volume, dst_volume=dst_volume
         )
         self.dest.volumes[self.arg] = await qubes.utils.coro_maybe(
-            dst_volume.import_volume(src_volume)
+            self.dest.storage.import_volume(dst_volume, src_volume)
         )
         self.app.save()
 
@@ -550,11 +550,9 @@ async def vm_volume_clear(self):
     )
     async def vm_volume_set_revisions_to_keep(self, untrusted_payload):
         self.enforce(self.arg in self.dest.volumes.keys())
-        newvalue = self.validate_size(untrusted_payload)
-
+        newvalue = self.validate_size(untrusted_payload, allow_negative=True)
         self.fire_event_for_permission(newvalue=newvalue)
-
-        self.dest.volumes[self.arg].revisions_to_keep = newvalue
+        self.dest.storage.set_revisions_to_keep(self.arg, newvalue)
         self.app.save()
 
     @qubes.api.method("admin.vm.volume.Set.rw", scope="local", write=True)
@@ -820,7 +818,10 @@ async def pool_set_revisions_to_keep(self, untrusted_payload):
         self.enforce(self.dest.name == "dom0")
         self.enforce(self.arg in self.app.pools.keys())
         pool = self.app.pools[self.arg]
-        newvalue = self.validate_size(untrusted_payload)
+        newvalue = self.validate_size(untrusted_payload, allow_negative=True)
+
+        if newvalue < -1:
+            raise qubes.api.ProtocolError("Invalid value for revisions_to_keep")
 
         self.fire_event_for_permission(newvalue=newvalue)
 

qubes/backup.py

@@ -517,10 +517,20 @@ def get_backup_summary(self):
             summary_line += fmt.format(size_to_human(vm_size))
 
             if qid != 0 and vm_info.vm.is_running():
-                summary_line += (
-                    " <-- The VM is running, backup will contain "
-                    "its state from before its start!"
-                )
+                if [
+                    volume
+                    for volume in vm_info.vm.volumes.values()
+                    if volume.snapshots_disabled
+                ]:
+                    summary_line += (
+                        " <-- The VM is running and private volume snapshots "
+                        "are disabled. Backup will fail!"
+                    )
+                else:
+                    summary_line += (
+                        " <-- The VM is running, backup will "
+                        "contain its state from before its start!"
+                    )
 
             summary += summary_line + "\n"
 
@@ -823,6 +833,15 @@ async def backup_do(self):
             backup_app.domains[qid].features["backup-content"] = True
             backup_app.domains[qid].features["backup-path"] = vm_info.subdir
             backup_app.domains[qid].features["backup-size"] = vm_info.size
+
+            # VM running private volumes without snapshoting them
+            # (revision_to_keep = -1) must be powered off to be backup
+            if backup_app.domains[qid].is_running:
+                for volume in backup_app.domains[qid].volumes.values():
+                    if volume.snapshots_disabled:
+                        raise qubes.exc.QubesVMNotHaltedError(
+                            backup_app.domains[qid]
+                        )
         backup_app.save()
         del backup_app
 

qubes/storage/__init__.py

@@ -41,6 +41,8 @@
 from qubes.exc import StoragePoolException
 
 STORAGE_ENTRY_POINT = "qubes.storage"
+VOLUME_STATE_DIR = "/var/run/qubes/"
+VOLUME_STATE_PREFIX = "volume-running-"
 _am_root = os.getuid() == 0
 
 BYTES_TO_ZERO = 1 << 16
@@ -96,7 +98,7 @@ def __init__(
         snap_on_start=False,
         source=None,
         ephemeral=None,
-        **kwargs
+        **kwargs,
     ):
         """Initialize a volume.
 
@@ -513,6 +515,27 @@ def _not_implemented(self, method_name):
         msg = msg.format(str(self.__class__.__name__), method_name)
         return NotImplementedError(msg)
 
+    @property
+    def snapshots_disabled(self) -> bool:
+        return (
+            self.revisions_to_keep == -1
+            and not self.snap_on_start
+            and self.save_on_stop
+        )
+
+    @property
+    def state_file(self) -> str:
+        return os.path.join(
+            VOLUME_STATE_DIR,
+            VOLUME_STATE_PREFIX
+            + f"{self.pool.name}:{self.vid}".replace("-", "--").replace(
+                "/", "-"
+            ),
+        )
+
+    def is_running(self) -> bool:
+        return os.path.exists(self.state_file)
+
 
 class Storage:
     """Class for handling VM virtual disks.
@@ -786,8 +809,37 @@ def block_devices(self):
                 else:
                     yield block_dev
 
+    def set_revisions_to_keep(self, volume, value):
+        if value < -1:
+            raise qubes.exc.QubesValueError(
+                "Invalid value for revisions_to_keep"
+            )
+
+        currentvalue = self.vm.volumes[volume].revisions_to_keep
+        enabling_disabling_snapshots = value != currentvalue and (
+            currentvalue == -1 or value == -1
+        )
+        if self.vm.is_running() and enabling_disabling_snapshots:
+            raise qubes.exc.QubesVMNotHaltedError(self.vm)
+
+        if self.vm.klass == "AppVM" and enabling_disabling_snapshots:
+            for vm in self.vm.dispvms:
+                if vm.is_running():
+                    raise qubes.exc.QubesVMNotHaltedError(vm)
+
+        self.vm.volumes[volume].revisions_to_keep = value
+
     async def start(self):
         """Execute the start method on each volume"""
+        for vol in self.vm.volumes.values():
+            if (
+                vol.source
+                and vol.source.snapshots_disabled
+                and vol.source.is_running()
+            ):
+                raise qubes.exc.QubesVMError(
+                    self.vm, f"Volume {vol.source.vid} is running"
+                )
         await qubes.utils.void_coros_maybe(
             # pylint: disable=line-too-long
             (
@@ -800,6 +852,10 @@ async def start(self):
             for name, vol in self.vm.volumes.items()
         )
 
+        for vol in self.vm.volumes.values():
+            with open(vol.state_file, "w", encoding="ascii"):
+                pass
+
     async def stop(self):
         """Execute the stop method on each volume"""
         await qubes.utils.void_coros_maybe(
@@ -813,6 +869,8 @@ async def stop(self):
             )
             for name, vol in self.vm.volumes.items()
         )
+        for vol in self.vm.volumes.values():
+            qubes.utils.remove_file(vol.state_file)
 
     def unused_frontend(self):
         """Find an unused device name"""
@@ -863,6 +921,18 @@ async def import_data_end(self, volume, success):
             self.get_volume(volume).import_data_end(success=success)
         )
 
+    async def import_volume(self, dst_volume: Volume, src_volume: Volume):
+        """Helper function to import data from another volume"""
+        if src_volume.is_running() and src_volume.snapshots_disabled:
+            raise StoragePoolException(
+                f"Volume {src_volume.vid} must be stopped before importing its "
+                f"data"
+            )
+
+        return await qubes.utils.coro_maybe(
+            dst_volume.import_volume(src_volume)
+        )
+
 
 class VolumesCollection:
     """Convenient collection wrapper for pool.get_volume and

qubes/storage/file.py

@@ -272,7 +272,7 @@ def revisions_to_keep(self):
 
     @revisions_to_keep.setter
     def revisions_to_keep(self, value):
-        if not isinstance(value, int) or value > 1 or value < 0:
+        if not isinstance(value, int) or value > 1 or value < -1:
             raise NotImplementedError(
                 "FileVolume supports maximum 1 volume revision to keep"
             )
@@ -490,9 +490,14 @@ async def start(self):
             # shutdown routine wasn't called (power interrupt or so)
             _remove_if_exists(self.path_cow)
         if not os.path.exists(self.path_cow):
-            create_sparse_file(self.path_cow, self.size)
+            if not self.snapshots_disabled:
+                create_sparse_file(self.path_cow, self.size)
         if not self.snap_on_start:
             _check_path(self.path)
+
+        if self.snapshots_disabled:
+            return self
+
         if hasattr(self, "path_source_cow"):
             if not os.path.exists(self.path_source_cow):
                 create_sparse_file(self.path_source_cow, self.size)
@@ -524,10 +529,12 @@ async def stop(self):
             self._export_lock is not FileVolume._marker_exported
         ), "trying to stop exported file volume?"
         if self.save_on_stop or self.snap_on_start:
-            await self._destroy_blockdev()
+            if not self.snapshots_disabled:
+                await self._destroy_blockdev()
         if self.save_on_stop:
             if self.rw:
-                self.commit()
+                if not self.snapshots_disabled:
+                    self.commit()
             else:
                 _remove_if_exists(self.path_cow)
         elif self.snap_on_start:
@@ -569,7 +576,7 @@ def script(self):
 
     def _block_device_path(self):
         if not self.snap_on_start:
-            if not self.save_on_stop:
+            if not self.save_on_stop or self.snapshots_disabled:
                 return self.path
             return "-".join(
                 [

qubes/storage/lvm.py

@@ -781,7 +781,10 @@ async def start(self):
         try:
             if self.snap_on_start or self.save_on_stop:
                 if not self.save_on_stop or not self.is_dirty():
-                    await self._snapshot()
+                    if self.snapshots_disabled and self.revisions:
+                        await self._remove_revisions(self.revisions)
+                    if not self.snapshots_disabled:
+                        await self._snapshot()
                 else:
                     await self._activate()
             else:
@@ -796,7 +799,8 @@ async def stop(self):
         changed = True
         try:
             if self.save_on_stop:
-                changed = await self._commit()
+                if not self.snapshots_disabled:
+                    changed = await self._commit()
             elif self.snap_on_start:
                 changed = await self._remove_if_exists(self._vid_snap)
             else:
@@ -829,9 +833,10 @@ def block_device(self):
         the libvirt XML template as <disk>.
         """
         if self.snap_on_start or self.save_on_stop:
-            snap_path = "/dev/mapper/" + self._vid_snap.replace(
-                "-", "--"
-            ).replace("/", "-")
+            vid = self.vid if self.snapshots_disabled else self._vid_snap
+            snap_path = "/dev/mapper/" + vid.replace("-", "--").replace(
+                "/", "-"
+            )
             return qubes.storage.BlockDevice(
                 snap_path, self.name, None, self.rw, self.domain, self.devtype
             )

qubes/storage/reflink.py

@@ -182,6 +182,8 @@ def __init__(self, *args, **kwargs):
     def _update_precache(self):
         _remove_file(self._path_precache)
         yield
+        if self.snapshots_disabled:
+            return
         _copy_file(self._path_clean, self._path_precache, copy_mtime=True)
 
     def _remove_stale_precache(self):
@@ -263,6 +265,13 @@ def is_dirty(self):
     @_coroutinized
     def start(self):  # pylint: disable=invalid-overridden-method
         self._remove_incomplete_images()
+        if self.snapshots_disabled:
+            self._prune_revisions(keep=0)
+            _remove_file(self._path_precache)
+            if not self.is_dirty():
+                _rename_file(self._path_clean, self._path_dirty)
+
+            return self
         if not self.is_dirty():
             if self.snap_on_start:
                 _remove_file(self._path_clean)
@@ -283,7 +292,10 @@ def start(self):  # pylint: disable=invalid-overridden-method
     @_coroutinized
     def stop(self):  # pylint: disable=invalid-overridden-method
         if self.is_dirty():
-            self._commit(self._path_dirty)
+            if self.snapshots_disabled:
+                _rename_file(self._path_dirty, self._path_clean)
+            else:
+                self._commit(self._path_dirty)
         elif not self.save_on_stop:
             if not self.snap_on_start:
                 self._size = self.size  # preserve manual resize of image