DispVM: Fix volume size check in is_preload_outdated

The type of size in volume_config depends on whether the qube object has
been loaded from xml (str) or created freshly (int). Due to this, in
almost all cases preloaded disposables were wrongly discarded. Use the
Volume objects in vm.volumes instead.

While at it also improve the return value to correctly report why it
thinks it's outdated (both for changed private size as well as outdated
volumes).

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

Author: ben-grande

qubes/tests/integ/dispvm.py

@@ -802,17 +802,23 @@ async def _test_018_preload_global(self):
         self.log_preload()
         logger.info("end")
 
-    def test_019_preload_refresh(self):
-        """Refresh preload on volume change."""
-        self.loop.run_until_complete(self._test_019_preload_refresh())
+    def test_019_preload_discard_outdated_volumes(self):
+        """Discard preload if volumes are outdated compared to its templates."""
+        self.loop.run_until_complete(
+            self._test_019_preload_discard_outdated_volumes()
+        )
 
-    async def _test_019_preload_refresh(self):
+    async def _test_019_preload_discard_outdated_volumes(self):
         logger.info("start")
         self.log_preload()
         preload_max = 1
 
         self.disp_base.features["preload-dispvm-max"] = str(preload_max)
         for qube in [self.disp_base, self.disp_base.template]:
+            logger.info(
+                "discard because of outdated volume originating from %s",
+                qube.name,
+            )
             await self.wait_preload(preload_max)
             old_preload = self.disp_base.get_feat_preload()
             await qube.start()
@@ -832,25 +838,50 @@ 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())
+    def test_020_preload_discard_outdated_volume_size(self):
+        """Discard preload if private size differs with disposable template."""
+        self.loop.run_until_complete(
+            self._test_020_preload_discard_outdated_volume_size()
+        )
+
+    async def _test_020_preload_discard_outdated_volume_size(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()
+        old_size = self.disp_base.volume_config["private"]["size"]
+        size = int(old_size) + 512
+        await self.disp_base.storage.resize("private", size)
+        self.app.save()
+        dispvm = await asyncio.wait_for(
+            qubes.vm.dispvm.DispVM.from_appvm(self.disp_base), 30
+        )
+        self.assertNotIn(dispvm.name, preload_dispvm)
+        await dispvm.cleanup()
+        logger.info("end")
+
+    def test_021_preload_discard_outdated_setting(self):
+        """Discard preload if properties differ with the disposable template."""
+        self.loop.run_until_complete(
+            self._test_021_preload_discard_outdated_setting()
+        )
 
-    async def _test_020_preload_discard_outdated(self):
+    async def _test_021_preload_discard_outdated_setting(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()
+        dispvm = await asyncio.wait_for(
+            qubes.vm.dispvm.DispVM.from_appvm(self.disp_base), 30
+        )
+        self.assertNotIn(dispvm.name, preload_dispvm)
+        await dispvm.cleanup()
+        await self.wait_preload(preload_max)
         logger.info("end")
 
     @unittest.skipUnless(which("xdotool"), "xdotool not installed")

qubes/tests/vm/dispvm.py

@@ -150,7 +150,7 @@ def test_000_from_appvm(self, mock_storage, mock_makedirs, mock_symlink):
         mock_symlink.assert_not_called()
 
     @mock.patch("qubes.storage.Storage")
-    def test_000_from_appvm_preload_reject_max(self, mock_storage):
+    def test_001_from_appvm_preload_reject_max(self, mock_storage):
         mock_storage.return_value.create.side_effect = self.mock_coro
         self.appvm.template_for_dispvms = True
         orig_getitem = self.app.domains.__getitem__
@@ -174,7 +174,7 @@ def test_000_from_appvm_preload_reject_max(self, mock_storage):
     @mock.patch("os.symlink")
     @mock.patch("os.makedirs")
     @mock.patch("qubes.storage.Storage")
-    def test_000_from_appvm_preload_use(
+    def test_002_from_appvm_preload_use(
         self,
         mock_storage,
         mock_makedirs,
@@ -223,10 +223,8 @@ 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.is_preload_outdated.return_value = {}
             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)
             )
@@ -244,7 +242,7 @@ def test_000_from_appvm_preload_use(
     @mock.patch("os.symlink")
     @mock.patch("os.makedirs")
     @mock.patch("qubes.storage.Storage")
-    def test_000_from_appvm_preload_fill_gap(
+    def test_003_from_appvm_preload_fill_gap(
         self,
         mock_storage,
         mock_makedirs,
@@ -295,7 +293,7 @@ def test_000_from_appvm_preload_fill_gap(
         mock_symlink.assert_not_called()
         mock_makedirs.assert_called_once()
 
-    def test_000_get_preload_max(self):
+    def test_004_get_preload_max(self):
         self.assertEqual(qubes.vm.dispvm.get_preload_max(self.appvm), None)
         self.appvm.features["supported-rpc.qubes.WaitForRunningSystem"] = True
         self.appvm.features["preload-dispvm-max"] = 1
@@ -306,7 +304,7 @@ def test_000_get_preload_max(self):
         self.adminvm.features["preload-dispvm-max"] = 2
         self.assertEqual(qubes.vm.dispvm.get_preload_max(self.adminvm), 2)
 
-    def test_000_get_preload_templates(self):
+    def test_005_get_preload_templates(self):
         get_preload_templates = qubes.vm.dispvm.get_preload_templates
         self.assertEqual(get_preload_templates(self.app), [])
         self.appvm.template_for_dispvms = True
@@ -338,14 +336,14 @@ def test_000_get_preload_templates(self):
             get_preload_templates(self.app), [self.appvm, self.appvm_alt]
         )
 
-    def test_001_from_appvm_reject_not_allowed(self):
+    def test_006_from_appvm_reject_not_allowed(self):
         with self.assertRaises(qubes.exc.QubesException):
             self.loop.run_until_complete(
                 qubes.vm.dispvm.DispVM.from_appvm(self.appvm)
             )
 
     @unittest.skip("test is broken")
-    def test_002_template_change(self):
+    def test_007_template_change(self):
         self.appvm.template_for_dispvms = True
         orig_getitem = self.app.domains.__getitem__
         with mock.patch.object(
@@ -372,7 +370,7 @@ def test_002_template_change(self):
             self.loop.run_until_complete(dispvm.kill())
             dispvm.template = self.appvm
 
-    def test_003_dvmtemplate_template_change(self):
+    def test_008_dvmtemplate_template_change(self):
         self.appvm.template_for_dispvms = True
         orig_domains = self.app.domains
         with mock.patch.object(
@@ -394,7 +392,7 @@ def test_003_dvmtemplate_template_change(self):
             with self.assertRaises(qubes.exc.QubesValueError):
                 self.appvm.template = qubes.property.DEFAULT
 
-    def test_004_dvmtemplate_allowed_change(self):
+    def test_009_dvmtemplate_allowed_change(self):
         self.appvm.template_for_dispvms = True
         orig_domains = self.app.domains
         with mock.patch.object(
@@ -660,23 +658,17 @@ def test_023_inherit_ephemeral(self, _mock_makedirs, _mock_symlink):
         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
+    def test_024_is_preload_outdated(self, _mock_makedirs, _mock_symlink):
+        self.app.pools["alternative"] = qubes.tests.vm.appvm.TestPool(
+            name="alternative"
+        )
         self.appvm.template_for_dispvms = True
-
+        self.loop.run_until_complete(self.template.create_on_disk())
+        self.loop.run_until_complete(
+            self.appvm.create_on_disk(pool="alternative")
+        )
         self.appvm.features["supported-rpc.qubes.WaitForRunningSystem"] = True
         self.appvm.features["preload-dispvm-max"] = "1"
         orig_getitem = self.app.domains.__getitem__
@@ -685,12 +677,6 @@ def test_024_is_preload_outdated(
         ) 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),
@@ -700,24 +686,35 @@ def test_024_is_preload_outdated(
                     ),
                 }
             )
-            dispvm = self.loop.run_until_complete(
-                qubes.vm.dispvm.DispVM.from_appvm(self.appvm, preload=True)
+            dispvm = self.app.add_new_vm(
+                qubes.vm.dispvm.DispVM, name="disp42", template=self.appvm
             )
-            dispvm.volume_config = self.appvm.volume_config
+            self.appvm.features["preload-dispvm"] = "disp42"
 
-        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.loop.run_until_complete(dispvm.create_on_disk())
 
-        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"]),
-        )
+        with mock.patch.object(
+            dispvm.volumes["root"], "is_outdated", return_value=False
+        ), mock.patch.object(
+            dispvm.volumes["private"], "is_outdated", return_value=False
+        ), mock.patch.object(
+            dispvm.volumes["volatile"], "is_outdated", return_value=False
+        ), mock.patch.object(
+            dispvm.volumes["kernel"], "is_outdated", return_value=False
+        ):
+            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

@@ -457,17 +457,14 @@ def is_preload_outdated(self) -> dict:
             return differed
 
         appvm = self.template
-        if (
-            self.volume_config["private"]["size"]
-            != appvm.volume_config["private"]["size"]
-        ):
-            differed["volumes"] = ["private"]
+        if self.volumes["private"].size != appvm.volumes["private"].size:
+            differed["volumes_size"] = ["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
+        for vol_name, vol in self.volumes.items():
+            if vol.is_outdated():
+                differed["volumes_outdated"] = [vol_name]
+                return differed
 
         appvm_props = appvm.property_dict()
         props = self.property_dict()