Set deferred netvm for paused clients on shutdown

The force feature exists for the sole reason of skipping deletion of the
original feature if it has the same value as the current netvm.

For: #722
Fixes: QubesOS/qubes-issues#10173
For: QubesOS/qubes-issues#1512

Author: ben-grande

qubes/ext/admin.py

@@ -29,6 +29,7 @@
 
 
 PROHIBITED_FEATURES = [
+    "deferred-netvm-force",
     "deferred-netvm-original",
     "preload-dispvm",
     "preload-dispvm-completed",

qubes/tests/__init__.py

@@ -1404,10 +1404,10 @@ def enter_keys_in_window(self, title, keys):
         ] + keys
         subprocess.check_call(command)
 
-    def shutdown_and_wait(self, vm, timeout=60):
+    def shutdown_and_wait(self, vm, force=False, timeout=60):
         try:
             self.loop.run_until_complete(
-                vm.shutdown(wait=True, timeout=timeout)
+                vm.shutdown(wait=True, force=force, timeout=timeout)
             )
         except qubes.exc.QubesException:
             name = vm.name

qubes/tests/integ/network.py

@@ -241,6 +241,9 @@ def _networking_paused_from_none_to_existent(
         self.assertEqual(
             self.testvm1.features.get("deferred-netvm-original", None), ""
         )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
+        )
         self.loop.run_until_complete(self.testvm1.unpause())
         self._run_cmd_and_log_output(self.testvm1, ip_deadline)
         self._run_cmd_and_log_output(self.testvm1, name_deadline)
@@ -254,8 +257,11 @@ def _networking_paused_from_none_to_existent(
             0,
             "Ping by name on " + test + " failed",
         )
-        self.assertEqual(
-            self.testvm1.features.get("deferred-netvm-original", None), None
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
         )
         self.shutdown_and_wait(self.testvm1)
 
@@ -273,6 +279,9 @@ def _networking_paused_from_existent_to_none(
             self.testvm1.features.get("deferred-netvm-original", None),
             self.testnetvm.name,
         )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
+        )
         self.loop.run_until_complete(self.testvm1.unpause())
         self.assertNotEqual(
             self.run_cmd(self.testvm1, ip),
@@ -284,8 +293,11 @@ def _networking_paused_from_existent_to_none(
             0,
             "Ping by name on " + test + " succeeded but should have failed",
         )
-        self.assertEqual(
-            self.testvm1.features.get("deferred-netvm-original", None), None
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
         )
         self.shutdown_and_wait(self.testvm1)
 
@@ -303,6 +315,9 @@ def _networking_paused_change_shutdown_old(
             self.testvm1.features.get("deferred-netvm-original", None),
             self.testnetvm2.name,
         )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
+        )
         self.shutdown_and_wait(self.testnetvm2)
         self.loop.run_until_complete(self.testvm1.unpause())
         self._run_cmd_and_log_output(self.testvm1, ip_deadline)
@@ -317,8 +332,11 @@ def _networking_paused_change_shutdown_old(
             0,
             "Ping by name on " + test + " failed",
         )
-        self.assertEqual(
-            self.testvm1.features.get("deferred-netvm-original", None), None
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
         )
         self.shutdown_and_wait(self.testvm1)
 
@@ -337,6 +355,9 @@ def _networking_paused_change_purge_old(
             self.testvm1.features.get("deferred-netvm-original", None),
             self.testnetvm2.name,
         )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
+        )
         self.loop.run_until_complete(self.testnetvm2.kill())
         del self.app.domains[self.testnetvm2]
         self.loop.run_until_complete(self.testnetvm2.remove_from_disk())
@@ -355,8 +376,84 @@ def _networking_paused_change_purge_old(
             0,
             "Ping by name on " + test + " failed",
         )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
+        )
+        self.shutdown_and_wait(self.testvm1)
+
+    def _networking_paused_restart_netvm(
+        self, ip, name, ip_deadline, name_deadline
+    ):
+        test = "restart netvm and unpause client"
+        self.log.critical(test)
+        print(test)
+        self.testvm1.netvm = self.testnetvm
+        self.loop.run_until_complete(self.start_vm(self.testvm1))
+        self.loop.run_until_complete(self.testvm1.pause())
+        self.shutdown_and_wait(self.testnetvm, force=True)
+        self.assertEqual(
+            self.testvm1.features.get("deferred-netvm-original", None),
+            self.testnetvm.name,
+        )
+        self.assertTrue(self.testvm1.features.get("deferred-netvm-force", None))
+        self.loop.run_until_complete(self.start_vm(self.testnetvm))
+        self.loop.run_until_complete(self.testvm1.unpause())
+        self._run_cmd_and_log_output(self.testvm1, ip_deadline)
+        self._run_cmd_and_log_output(self.testvm1, name_deadline)
+        self.assertEqual(
+            self.run_cmd(self.testvm1, ip),
+            0,
+            "Ping by IP on " + test + " failed",
+        )
+        self.assertEqual(
+            self.run_cmd(self.testvm1, name),
+            0,
+            "Ping by name on " + test + " failed",
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
+        )
+        self.shutdown_and_wait(self.testvm1)
+
+    def _networking_paused_shutdown_netvm(
+        self, ip, name, ip_deadline, name_deadline
+    ):
+        test = "shutdown netvm and unpause client"
+        self.log.critical(test)
+        print(test)
+        self.testvm1.netvm = self.testnetvm
+        self.loop.run_until_complete(self.start_vm(self.testvm1))
+        self.loop.run_until_complete(self.testvm1.pause())
+        self.shutdown_and_wait(self.testnetvm, force=True)
+        self.assertEqual(
+            self.testvm1.features.get("deferred-netvm-original", None),
+            self.testnetvm.name,
+        )
+        self.assertTrue(self.testvm1.features.get("deferred-netvm-force", None))
+        self.loop.run_until_complete(self.testvm1.unpause())
+        self._run_cmd_and_log_output(self.testvm1, ip_deadline)
+        self._run_cmd_and_log_output(self.testvm1, name_deadline)
+        self.assertEqual(
+            self.run_cmd(self.testvm1, ip),
+            0,
+            "Ping by IP on " + test + " failed",
+        )
         self.assertEqual(
-            self.testvm1.features.get("deferred-netvm-original", None), None
+            self.run_cmd(self.testvm1, name),
+            0,
+            "Ping by name on " + test + " failed",
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-force", None)
         )
         self.shutdown_and_wait(self.testvm1)
 
@@ -406,6 +503,28 @@ def test_001_simple_networking_paused_change_purge_old(self):
             self.ping_deadline_name,
         )
 
+    def test_001_simple_networking_paused_restart_netvm(self):
+        """
+        :type self: qubes.tests.SystemTestCase | VmNetworkingMixin
+        """
+        self._networking_paused_restart_netvm(
+            self.ping_ip,
+            self.ping_name,
+            self.ping_deadline_ip,
+            self.ping_deadline_name,
+        )
+
+    def test_001_simple_networking_paused_shutdown_netvm(self):
+        """
+        :type self: qubes.tests.SystemTestCase | VmNetworkingMixin
+        """
+        self._networking_paused_shutdown_netvm(
+            self.ping_ip,
+            self.ping_name,
+            self.ping_deadline_ip,
+            self.ping_deadline_name,
+        )
+
     def test_010_simple_proxyvm(self):
         """
         :type self: qubes.tests.SystemTestCase | VmNetworkingMixin

qubes/tests/integ/network_ipv6.py

@@ -167,6 +167,28 @@ def test_501_simple_networking_paused_change_purge_old(self):
             self.ping6_deadline_name,
         )
 
+    def test_501_simple_networking_paused_restart_netvm(self):
+        """
+        :type self: qubes.tests.SystemTestCase | VmIPv6NetworkingMixin
+        """
+        self._networking_paused_restart_netvm(
+            self.ping6_ip,
+            self.ping6_name,
+            self.ping6_deadline_ip,
+            self.ping6_deadline_name,
+        )
+
+    def test_501_simple_networking_paused_shutdown_netvm(self):
+        """
+        :type self: qubes.tests.SystemTestCase | VmIPv6NetworkingMixin
+        """
+        self._networking_paused_shutdown_netvm(
+            self.ping6_ip,
+            self.ping6_name,
+            self.ping6_deadline_ip,
+            self.ping6_deadline_name,
+        )
+
     def test_510_ipv6_simple_proxyvm(self):
         """
         :type self: qubes.tests.SystemTestCase | VmIPv6NetworkingMixin

qubes/vm/dispvm.py

@@ -749,18 +749,18 @@ async def use_preload(self) -> None:
             self.log.info("Using preloaded qube")
             if not appvm.features.get("internal", None):
                 del self.features["internal"]
-            await self.apply_deferred_netvm()
             self.preload_requested = False
+            await self.apply_deferred_netvm()
             del self.features["preload-dispvm-in-progress"]
         else:
             # Happens when unpause/resume occurs without qube being requested.
             self.log.warning("Using a preloaded qube before requesting it")
             if not appvm.features.get("internal", None):
                 del self.features["internal"]
-            await self.apply_deferred_netvm()
             appvm.remove_preload_from_list(
                 [self.name], reason="qube was used without being requested"
             )
+            await self.apply_deferred_netvm()
             self.features["preload-dispvm-in-progress"] = False
         self.app.save()
         asyncio.ensure_future(

qubes/vm/mix/net.py

@@ -315,17 +315,24 @@ def on_domain_shutdown_net(self, event, **kwargs):
         This will allow re-reconnecting them cleanly later.
         """
         # pylint: disable=unused-argument
+        for feature in ["deferred-netvm-original", "deferred-netvm-force"]:
+            deferred_feat = self.features.get(feature, None)
+            if deferred_feat is None:
+                continue
+            del self.features[feature]
         for vm in self.connected_vms:
             if not vm.is_running():
                 continue
             try:
                 vm.detach_network()
+                if vm.is_paused():
+                    vm.features["deferred-netvm-force"] = True
             except (qubes.exc.QubesException, libvirt.libvirtError):
                 # ignore errors
                 pass
 
     @qubes.events.handler("domain-start")
-    def on_domain_started_net(self, event, **kwargs):
+    async def on_domain_started_net(self, event, **kwargs):
         """Connect this domain to its downstream domains. Also reload firewall
         in its netvm.
 
@@ -336,11 +343,15 @@ def on_domain_started_net(self, event, **kwargs):
             self.netvm.reload_firewall_for_vm(self)  # pylint: disable=no-member
 
         for vm in self.connected_vms:
-            if not vm.is_running():
+            if not vm.is_running() or vm.is_paused():
                 continue
             vm.log.info("Attaching network")
             try:
-                vm.attach_network()
+                deferred_from = vm.features.get("deferred-netvm-original", None)
+                if deferred_from is not None:
+                    await vm.apply_deferred_netvm()
+                else:
+                    vm.attach_network()
             except (qubes.exc.QubesException, libvirt.libvirtError):
                 vm.log.warning("Cannot attach network", exc_info=1)
 
@@ -358,6 +369,11 @@ async def apply_deferred_netvm(self):
         oldvalue = None
         if deferred_from in self.app.domains:
             oldvalue = self.app.domains[deferred_from]
+        if self.netvm and not self.netvm.is_running():
+            # Too early to create routing table. Happens when shutting down
+            # netvm and not starting it before attempting to use the client.
+            # This function will be called again by domain-start of the netvm.
+            return
         self.fire_event(
             "property-pre-set:netvm",
             pre_event=True,
@@ -373,6 +389,9 @@ async def apply_deferred_netvm(self):
                 oldvalue=oldvalue,
             )
         del self.features["deferred-netvm-original"]
+        deferred_force = self.features.get("deferred-netvm-force", None)
+        if deferred_force is not None:
+            del self.features["deferred-netvm-force"]
         # Guarantee that uplink has been concluded (established or absent).
         await asyncio.wait_for(
             self.run_service_for_stdio(
@@ -442,22 +461,32 @@ def detach_network(self):
 
         if not self.is_running():
             raise qubes.exc.QubesVMNotRunningError(self)
+        deferred_from = self.features.get("deferred-netvm", None)
         if self.netvm is None:
-            deferred_from = self.features.get("deferred-netvm", None)
             if deferred_from is not None:
                 raise qubes.exc.QubesVMError(
                     self, "netvm should not be {}".format(self.netvm)
                 )
 
+        # Properties extracted from libvirt_domain to support deferred netvm.
+        root = lxml.etree.fromstring(self.libvirt_domain.XMLDesc())
+        eth = root.find(".//interface[@type='ethernet']")
+
         if self.is_paused():
             self.log.warning(
                 "Deferred detaching libvirt net device because qube is paused"
             )
+            if deferred_from is not None or eth is None:
+                return
+            backend = eth.find("backenddomain")
+            if backend is None:
+                return
+            backend_name = backend.get("name")
+            if backend_name not in self.app.domains:
+                return
+            self.features["deferred-netvm-original"] = str(backend_name)
             return
 
-        # Properties extracted from libvirt_domain to support deferred netvm.
-        root = lxml.etree.fromstring(self.libvirt_domain.XMLDesc())
-        eth = root.find(".//interface[@type='ethernet']")
         if eth is None:
             return
         self.libvirt_domain.detachDevice(lxml.etree.tostring(eth).decode())
@@ -587,10 +616,11 @@ def on_property_pre_set_netvm(self, event, name, newvalue, oldvalue=None):
                 )
 
         deferred_from = self.features.get("deferred-netvm-original", None)
-        if deferred_from is not None and (
-            (not deferred_from and not newvalue)
-            or (newvalue and deferred_from == newvalue.name)
-        ):
+        deferred_force = self.features.get("deferred-netvm-force", None)
+        same_name = (not deferred_from and not newvalue) or (
+            newvalue and deferred_from == newvalue.name
+        )
+        if not deferred_force and deferred_from is not None and same_name:
             # No deferred netvm as original value is restored.
             del self.features["deferred-netvm-original"]
             return