Avoid hanging qubesd on unresponsive domU shutdown

The action blocks either way as we have to await for them to complete,
but the "xl" command does not block qubesd while "virsh" does, as it has
a global lock.

For: QubesOS/qubes-issues#10648
For: QubesOS/qubes-issues#10074

Author: ben-grande

qubes/tests/vm/mix/net.py

@@ -20,6 +20,7 @@
 # License along with this library; if not, see <https://www.gnu.org/licenses/>.
 #
 import ipaddress
+from unittest import mock
 from unittest.mock import patch
 
 import qubes
@@ -395,7 +396,18 @@ def test_180_shutdown(self, mock_halted, mock_shutdown):
             with self.assertRaises(qubes.exc.QubesVMInUseError):
                 self.loop.run_until_complete(vm.netvm.shutdown())
             vm.is_preload = True
-            self.loop.run_until_complete(vm.netvm.shutdown())
+            mock_proc = mock.AsyncMock()
+            mock_proc.communicate.return_value = (b"", None)
+            mock_proc.wait.return_value = 0
+            mock_proc.returncode = 0
+            with (
+                patch.object(vm.app, "vmm") as mock_vmm,
+                patch(
+                    "asyncio.create_subprocess_exec", return_value=mock_proc
+                ) as mock_async_sub,
+            ):
+                mock_vmm.libvirt_conn.getType.return_value = "Xen"
+                self.loop.run_until_complete(vm.netvm.shutdown())
 
     def test_200_vmid_to_ipv4(self):
         testcases = (

qubes/tests/vm/qubesvm.py

@@ -2950,6 +2950,74 @@ def test_626_qdb_keyboard_layout_change(
     async def coroutine_mock(self, mock, *args, **kwargs):
         return mock(*args, **kwargs)
 
+    @unittest.mock.patch("asyncio.create_subprocess_exec")
+    @unittest.mock.patch(
+        "qubes.vm.qubesvm.QubesVM.is_running", return_value=True
+    )
+    @unittest.mock.patch("qubes.vm.qubesvm.QubesVM.libvirt_domain")
+    @unittest.mock.patch(
+        "qubes.vm.qubesvm.QubesVM.is_halted", return_value=False
+    )
+    def test_650_shutdown(
+        self, mock_halted, mock_shutdown, mock_running, mock_async_sub
+    ):
+        # pylint: disable=unused-argument
+        vm = self.get_vm()
+        mock_proc = unittest.mock.AsyncMock()
+        mock_proc.communicate.return_value = (b"", None)
+        mock_proc.returncode = 0
+        mock_proc.wait.return_value = mock_proc.returncode
+        mock_async_sub.return_value = mock_proc
+        with unittest.mock.patch.object(vm.app, "vmm") as mock_vmm:
+            with self.subTest("xen"):
+                mock_vmm.libvirt_conn.getType.return_value = "Xen"
+                self.loop.run_until_complete(vm.shutdown())
+                mock_async_sub.assert_called_once_with(
+                    "xl",
+                    "shutdown",
+                    "-F",
+                    vm.name,
+                    stdin=unittest.mock.ANY,
+                    stdout=unittest.mock.ANY,
+                    stderr=unittest.mock.ANY,
+                )
+
+            mock_async_sub.reset_mock()
+            with self.subTest("xen-fail"):
+                mock_proc.communicate.return_value = (b"Oh no (xen)", None)
+                mock_proc.returncode = 1
+                mock_vmm.libvirt_conn.getType.return_value = "Xen"
+                with self.assertRaises(qubes.exc.QubesVMShutdownTimeoutError):
+                    self.loop.run_until_complete(vm.shutdown())
+
+            mock_async_sub.reset_mock()
+            with self.subTest("not-xen"):
+                mock_proc.returncode = 0
+                uri = "kvm:///"
+                mock_vmm.libvirt_conn.getType.return_value = "NotXen"
+                mock_vmm.libvirt_conn.getURI.return_value = uri
+                self.loop.run_until_complete(vm.shutdown())
+                mock_async_sub.assert_called_once_with(
+                    "virsh",
+                    "-c",
+                    uri,
+                    "shutdown",
+                    vm.name,
+                    stdin=unittest.mock.ANY,
+                    stdout=unittest.mock.ANY,
+                    stderr=unittest.mock.ANY,
+                )
+
+            mock_async_sub.reset_mock()
+            with self.subTest("not-xen-fail"):
+                mock_proc.communicate.return_value = (b"Oh no (not-xen)", None)
+                mock_proc.returncode = 1
+                uri = "kvm:///"
+                mock_vmm.libvirt_conn.getType.return_value = "NotXen"
+                mock_vmm.libvirt_conn.getURI.return_value = uri
+                with self.assertRaises(qubes.exc.QubesVMShutdownTimeoutError):
+                    self.loop.run_until_complete(vm.shutdown())
+
     @unittest.mock.patch("asyncio.create_subprocess_exec")
     def test_700_run_service(self, mock_subprocess):
         start_mock = unittest.mock.AsyncMock()

qubes/vm/qubesvm.py

@@ -1,3 +1,4 @@
+# pylint; disable=too-many-lines
 #
 # The Qubes OS Project, https://www.qubes-os.org/
 #
@@ -1661,14 +1662,50 @@ async def shutdown(self, force=False, wait=False, timeout=None):
             if self.is_paused():
                 self.libvirt_domain.destroy()
             else:
+                action = "shutdown"
+                timeout_exc = qubes.exc.QubesVMShutdownTimeoutError(self)
+                # Some libvirt actions have a global lock on a domain, blocking
+                # a lot of operations and even qubesd. When possible to act
+                # without it, do so to avoid the whole qubesd hanging.
+                if self.app.vmm.libvirt_conn.getType() == "Xen":
+                    command = ["xl", action, "-F", self.name]
+                else:
+                    # or: self.libvirt_domain.shutdown() without subprocess
+                    uri = self.app.vmm.libvirt_conn.getURI()
+                    command = ["virsh", "-c", uri, action, self.name]
                 try:
-                    self.libvirt_domain.shutdown()
+                    proc = await asyncio.create_subprocess_exec(
+                        *command,
+                        stdin=asyncio.subprocess.DEVNULL,
+                        stdout=asyncio.subprocess.PIPE,
+                        stderr=asyncio.subprocess.STDOUT,
+                    )
+                    stdout, _ = await proc.communicate()
+                    await proc.wait()
+                    if proc.returncode:
+                        raise subprocess.CalledProcessError(
+                            proc.returncode,
+                            command,
+                            output=stdout,
+                        )
+                except subprocess.CalledProcessError as e:
+                    self.log.error(
+                        "Attempted {!s} with subprocess but exited with "
+                        "error code: {!s}: {!r}".format(
+                            action,
+                            e.returncode,
+                            qubes.utils.sanitize_stderr_for_log(e.output),
+                        )
+                    )
+                    raise timeout_exc
                 except libvirt.libvirtError as e:
-                    if e.get_error_code() == libvirt.VIR_ERR_INTERNAL_ERROR:
-                        raise qubes.exc.QubesVMShutdownTimeoutError(self)
-                    self.log.exception(
-                        "libvirt error code: {!r}".format(e.get_error_code())
+                    exit_code = e.get_error_code()
+                    self.log.error(
+                        "Attempted {!s} with libvirt but exited with error "
+                        "code: {!s}".format(action, exit_code)
                     )
+                    if exit_code == libvirt.VIR_ERR_INTERNAL_ERROR:
+                        raise timeout_exc
                     raise
 
             if wait: