Discard preloaded disposables with outdated props

If a qube property is changed on the disposable template, it is not
replicated to the preloaded disposable, no refresh occurs, which means,
if a disposable template has the netvm changed, the preloaded disposable
would still remain with the old setting. While changing netvm on the fly
is possible after unpause, several other settings requires a restart.

Fixes: QubesOS/qubes-issues#10525
For: QubesOS/qubes-issues#1512

Author: ben-grande

qubes/tests/integ/dispvm.py

@@ -832,8 +832,29 @@ async def _test_019_preload_refresh(self):
         self.log_preload()
         logger.info("end")
 
+    def test_020_preload_discard_outdated(self):
+        """Discard preload if properties differ from the disposable template."""
+        self.loop.run_until_complete(self._test_020_preload_discard_outdated())
+
+    async def _test_020_preload_discard_outdated(self):
+        logger.info("start")
+        self.log_preload()
+        preload_max = 1
+        self.disp_base.features["preload-dispvm-max"] = str(preload_max)
+        await self.wait_preload(preload_max)
+        preload_dispvm = self.disp_base.get_feat_preload()
+        self.disp_base.netvm = None
+        try:
+            dispvm = await asyncio.wait_for(
+                qubes.vm.dispvm.DispVM.from_appvm(self.disp_base), 30
+            )
+            self.assertNotIn(dispvm.name, preload_dispvm)
+        finally:
+            await dispvm.cleanup()
+        logger.info("end")
+
     @unittest.skipUnless(which("xdotool"), "xdotool not installed")
-    def test_020_gui_app(self):
+    def test_080_gui_app(self):
         dispvm = self.loop.run_until_complete(
             qubes.vm.dispvm.DispVM.from_appvm(self.disp_base)
         )
@@ -1057,7 +1078,7 @@ def _whonix_ws_dispvm_confirm(self, action_str):
         return (True, "")
 
     @unittest.skipUnless(which("xdotool"), "xdotool not installed")
-    def test_030_edit_file(self):
+    def test_090_edit_file(self):
         self.testvm1 = self.app.add_new_vm(
             qubes.vm.appvm.AppVM,
             name=self.make_vm_name("vm1"),

qubes/tests/vm/dispvm.py

@@ -181,6 +181,7 @@ def test_000_from_appvm_preload_use(
         mock_symlink,
         mock_start,
     ):
+        # pylint: disable=unused-argument
         mock_storage.return_value.create.side_effect = self.mock_coro
         mock_start.side_effect = self.mock_coro
         self.appvm.template_for_dispvms = True
@@ -222,8 +223,10 @@ def test_000_from_appvm_preload_use(
             mock_qube.features = dispvm.features
             mock_qube.unpause = self.mock_coro
             mock_qube.request_preload.return_value = dispvm
+            mock_qube.is_preload_outdated = dispvm.is_preload_outdated
             mock_qube.get_preload = mock.AsyncMock()
             mock_qube.volumes = {}
+            dispvm.volume_config = self.appvm.volume_config
             fresh_dispvm = self.loop.run_until_complete(
                 qubes.vm.dispvm.DispVM.from_appvm(self.appvm)
             )
@@ -656,3 +659,65 @@ def test_023_inherit_ephemeral(self, _mock_makedirs, _mock_symlink):
             self.loop.run_until_complete(dispvm.create_on_disk())
         self.assertIs(dispvm.template, self.appvm)
         self.assertTrue(dispvm.volumes["volatile"].ephemeral)
+
+    @mock.patch("qubes.vm.qubesvm.QubesVM.start")
+    @mock.patch("os.symlink")
+    @mock.patch("os.makedirs")
+    @mock.patch("qubes.storage.Storage")
+    def test_024_is_preload_outdated(
+        self,
+        mock_storage,
+        mock_makedirs,
+        mock_symlink,
+        mock_start,
+    ):
+        mock_storage.return_value.create.side_effect = self.mock_coro
+        mock_makedirs.return_value = self.mock_coro
+        mock_symlink.return_value = self.mock_coro
+        mock_start.side_effect = self.mock_coro
+        self.appvm.template_for_dispvms = True
+
+        self.appvm.features["supported-rpc.qubes.WaitForRunningSystem"] = True
+        self.appvm.features["preload-dispvm-max"] = "1"
+        orig_getitem = self.app.domains.__getitem__
+        with mock.patch.object(
+            self.app, "domains", wraps=self.app.domains
+        ) as mock_domains:
+            mock_qube = mock.Mock()
+            mock_qube.template = self.appvm
+            mock_qube.qrexec_timeout = self.appvm.qrexec_timeout
+            mock_qube.preload_complete = mock.Mock(spec=asyncio.Event)
+            mock_qube.preload_complete.is_set.return_value = True
+            mock_qube.preload_complete.set = self.mock_coro
+            mock_qube.preload_complete.clear = self.mock_coro
+            mock_qube.preload_complete.wait = self.mock_coro
+            mock_domains.configure_mock(
+                **{
+                    "get_new_unused_dispid": mock.Mock(return_value=42),
+                    "__contains__.return_value": True,
+                    "__getitem__.side_effect": lambda key: (
+                        mock_qube if key == "disp42" else orig_getitem(key)
+                    ),
+                }
+            )
+            dispvm = self.loop.run_until_complete(
+                qubes.vm.dispvm.DispVM.from_appvm(self.appvm, preload=True)
+            )
+            dispvm.volume_config = self.appvm.volume_config
+
+        self.assertFalse(dispvm.is_preload_outdated())
+        self.appvm.debug = not self.appvm.debug
+        self.assertEqual(
+            dispvm.is_preload_outdated(), {"properties": ["debug"]}
+        )
+        self.appvm.debug = not self.appvm.debug
+
+        self.assertFalse(dispvm.is_preload_outdated())
+        self.appvm_alt.provides_network = True
+        self.assertFalse(dispvm.is_preload_outdated())
+        self.appvm.netvm = self.appvm_alt
+        self.assertIn("properties", dispvm.is_preload_outdated().keys())
+        self.assertEqual(
+            sorted(dispvm.is_preload_outdated()["properties"]),
+            sorted(["netvm", "dns", "visible_netmask"]),
+        )

qubes/vm/dispvm.py

@@ -31,6 +31,35 @@
 import qubes.vm.appvm
 import qubes.vm.qubesvm
 
+PRELOAD_OUTDATED_IGNORED_PROPERTIES = [
+    "autostart",
+    "backup_timestamp",
+    "default_dispvm",
+    "dispid",
+    "gateway",
+    "gateway6",
+    "icon",
+    "include_in_backups",
+    "installed_by_rpm",
+    "ip",
+    "ip6",
+    "klass",
+    "name",
+    "qid",
+    "start_time",
+    "stubdom_uuid",
+    "stubdom_xid",
+    "template",
+    "template_for_dispvms",
+    "updateable",
+    "uuid",
+    "visible_gateway",
+    "visible_gateway6",
+    "visible_ip",
+    "visible_ip6",
+    "xid",
+]
+
 
 def _setter_template(self, prop, value):
     if not getattr(value, "template_for_dispvms", False):
@@ -417,6 +446,43 @@ def is_preload(self) -> bool:
             return True
         return False
 
+    def is_preload_outdated(self) -> dict:
+        """
+        Show properties that differ on disposable compared to its template.
+
+        :rtype: dict
+        """
+        differed: dict[str, list] = {}
+        if not self.is_preload:
+            return differed
+
+        appvm = self.template
+        if (
+            self.volume_config["private"]["size"]
+            != appvm.volume_config["private"]["size"]
+        ):
+            differed["volumes"] = ["private"]
+            return differed
+
+        if any(vol for vol in self.volumes.values() if vol.is_outdated()):
+            # Volume name is irrelevant. We use any() to return fast.
+            differed["volumes"] = ["root"]
+            return differed
+
+        appvm_props = appvm.property_dict()
+        props = self.property_dict()
+        differed_props = [
+            k
+            for k in props.keys() & appvm_props.keys()
+            if k not in PRELOAD_OUTDATED_IGNORED_PROPERTIES
+            and getattr(self, k, None) != getattr(appvm, k, None)
+        ]
+        if not differed_props:
+            return differed
+        # Not using any() cause it is nice to know the property for debugging.
+        differed["properties"] = differed_props
+        return differed
+
     @qubes.events.handler("domain-load")
     def on_domain_loaded(self, event) -> None:
         """

qubes/vm/mix/dvmtemplate.py

@@ -148,7 +148,15 @@ async def on_dvmtemplate_domain_shutdown(self, _event, **_kwargs) -> None:
         """
         Refresh preloaded disposables on shutdown.
         """
-        await self.refresh_preload()
+        self.refresh_outdated_preload()
+
+    @qubes.events.handler("property-reset:*", "property-set:*")
+    def on_dvmtemplate_property_changed(self, _event, name, **_kwargs) -> None:
+        """
+        Refresh preloaded disposables if property affects the disposable.
+        """
+        if name not in qubes.vm.dispvm.PRELOAD_OUTDATED_IGNORED_PROPERTIES:
+            self.refresh_outdated_preload(delay=30)
 
     @qubes.events.handler("domain-feature-pre-set:preload-dispvm-delay")
     def on_feature_pre_set_preload_dispvm_delay(
@@ -641,21 +649,32 @@ def can_preload(self) -> bool:
             return True
         return False
 
-    async def refresh_preload(self) -> None:
+    def refresh_outdated_preload(
+        self, skip_check: bool = False, delay: Union[int, float] = 4
+    ) -> None:
         """
-        Refresh disposables which have outdated volumes.
+        Refresh disposables which have outdated volumes or properties.
+
+        :param bool skip_check: Skip check of outdated preloads, refresh all.
         """
         assert isinstance(self, qubes.vm.BaseVM)
-        outdated = []
-        for qube in self.dispvms:
-            if not qube.is_preload or not any(
-                vol.is_outdated() for vol in qube.volumes.values()
-            ):
+        outdated: list | Iterator = []
+        dispvms: list | Iterator = []
+        if skip_check:
+            outdated = self.dispvms
+        else:
+            dispvms = self.dispvms
+        for qube in dispvms:
+            if not qube.is_preload:
                 continue
-            outdated.append(qube)
+            if qube.is_preload_outdated():
+                assert isinstance(outdated, list)
+                outdated.append(qube)
+
         if outdated:
             self.remove_preload_from_list(
-                [qube.name for qube in outdated], reason="of outdated volume(s)"
+                [qube.name for qube in outdated],
+                reason="of outdated volume(s) or property(ies)",
             )
             tasks = [self.app.domains[qube].cleanup() for qube in outdated]
             asyncio.ensure_future(asyncio.gather(*tasks))
@@ -664,7 +683,7 @@ async def refresh_preload(self) -> None:
                 self.fire_event_async(
                     "domain-preload-dispvm-start",
                     reason="of outdated volume(s)",
-                    delay=4,
+                    delay=delay,
                 )
             )
 
@@ -682,16 +701,20 @@ def request_preload(self) -> Optional["qubes.vm.dispvm.DispVM"]:
         dispvm = None
         for item in preload_dispvm:
             qube = self.app.domains[item]
-            if any(vol.is_outdated() for vol in qube.volumes.values()):
+            if outdated_reason := qube.is_preload_outdated():
+                if "properties" in outdated_reason:
+                    discard_reason = "property(ies): " + ", ".join(
+                        map(str, outdated_reason["properties"])
+                    )
+                else:
+                    discard_reason = "volume(s)"
                 qube.log.warning(
-                    "Requested preloaded qube but it is outdated, trying "
-                    "another one if available"
+                    "Discarding preloaded disposable as it has has outdated %s",
+                    discard_reason,
                 )
-                # The gap is filled after the delay set by the
-                # 'domain-shutdown' of its ancestors. Not refilling now to
-                # deliver a disposable faster.
+                # Not refilling now to deliver a disposable faster.
                 self.remove_preload_from_list(
-                    [qube.name], reason="of outdated volume(s)"
+                    [qube.name], reason="of outdated " + discard_reason
                 )
                 # Delay to not  affect this run.
                 asyncio.ensure_future(
@@ -706,6 +729,7 @@ def request_preload(self) -> Optional["qubes.vm.dispvm.DispVM"]:
                 "Found only outdated preloaded qube(s), falling back to "
                 "normal disposable"
             )
+            self.fill_preload_gap()
             return None
         dispvm.mark_preload_requested()
         return dispvm

qubes/vm/templatevm.py

@@ -116,12 +116,11 @@ async def on_template_domain_shutdown(self, _event, **_kwargs):
         """
         appvms = [
             qube
-            for qube in self.app.domains
-            if getattr(qube, "template", None) == self
-            and getattr(qube, "template_for_dispvms", False)
+            for qube in self.appvms
+            if getattr(qube, "template_for_dispvms", False)
         ]
         for qube in appvms:
-            await qube.refresh_preload()
+            qube.refresh_outdated_preload()
 
     @qubes.events.handler("domain-feature-set:boot-mode.appvm-default")
     def on_feature_bootmode_appvm_set(