Patchwork xen: Avoid deadlock in xenUnifiedDomainGetXMLDesc

login
register
mail settings
Submitter Stefan Bader
Date June 24, 2013, 8:57 a.m.
Message ID <1372064255-2421-1-git-send-email-stefan.bader@canonical.com>
Download mbox | patch
Permalink /patch/253742/
State New
Headers show

Comments

Stefan Bader - June 24, 2013, 8:57 a.m.
So this is one way I am able to get this issue resolved. It might not
be ideal as maybe there is a slight chance of inconsistencies. But at
least it does not lock up anymore.

-Stefan

---

From f0c28a9f7af84a01bb2c3d71067adefb92e919d9 Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Mon, 24 Jun 2013 09:30:20 +0200
Subject: [PATCH] xen: Avoid deadlock in xenUnifiedDomainGetXMLDesc

Commit 95e18efd added the use virDomainDefPtr to several Xen
driver methods. That structure is obtained by calling
xenGetDomainDefForDom() which will acquire the mutex to the
priv pointer.
Unfortunately (at least) xenUnifiedDomainGetXMLDesc already
takes that mutex and now is locking up while calling into
xenDomainUsedCpus().

xenDomainUsedCpus
  ...
  nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
    return xenUnifiedDomainGetVcpusFlags(...)
      ...
      if (!(def = xenGetDomainDefForDom(dom)))
        return xenGetDomainDefForUUID(dom->conn, dom->uuid);
          ...
          ret = xenHypervisorLookupDomainByUUID(conn, uuid);
            ...
            xenUnifiedLock(priv);
            name = xenStoreDomainGetName(conn, id);
            xenUnifiedUnlock(priv);
  ...
  if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
    ...
    if (!(def = xenGetDomainDefForDom(dom)))
      [again like above]

The deadlock is observable when connecting to a Xen host using the
old xm stack and trying to obtain domain XML data from any running
guest (like "dumpxml 0").

This would be a minimal fix that tries to avoid the deadlock. I am not
completely sure this is not allowing a small window for getting
inconsistent data.

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 src/xen/xen_driver.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
Stefan Bader - June 24, 2013, 9:05 a.m.
Arg, I am no awake yet. This should have gone to a completely different list.

-Stefan

Patch

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 3efc27a..64cc305 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -189,6 +189,7 @@  xenDomainUsedCpus(virDomainPtr dom)
     unsigned char *cpumap = NULL;
     size_t cpumaplen;
     int nb = 0;
+    int nbNodeCpus;
     int n, m;
     virVcpuInfoPtr cpuinfo = NULL;
     virNodeInfo nodeinfo;
@@ -198,8 +199,11 @@  xenDomainUsedCpus(virDomainPtr dom)
         return NULL;
 
     priv = dom->conn->privateData;
+    xenUnifiedLock(priv);
+    nbNodeCpus = priv->nbNodeCpus;
+    xenUnifiedUnlock(priv);
 
-    if (priv->nbNodeCpus <= 0)
+    if (nbNodeCpus <= 0)
         return NULL;
     nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
     if (nb_vcpu <= 0)
@@ -207,7 +211,7 @@  xenDomainUsedCpus(virDomainPtr dom)
     if (xenUnifiedNodeGetInfo(dom->conn, &nodeinfo) < 0)
         return NULL;
 
-    if (!(cpulist = virBitmapNew(priv->nbNodeCpus))) {
+    if (!(cpulist = virBitmapNew(nbNodeCpus))) {
         virReportOOMError();
         goto done;
     }
@@ -225,7 +229,7 @@  xenDomainUsedCpus(virDomainPtr dom)
     if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
                                           cpumap, cpumaplen)) >= 0) {
         for (n = 0; n < ncpus; n++) {
-            for (m = 0; m < priv->nbNodeCpus; m++) {
+            for (m = 0; m < nbNodeCpus; m++) {
                 bool used;
                 ignore_value(virBitmapGetBit(cpulist, m, &used));
                 if ((!used) &&
@@ -233,7 +237,7 @@  xenDomainUsedCpus(virDomainPtr dom)
                     ignore_value(virBitmapSetBit(cpulist, m));
                     nb++;
                     /* if all CPU are used just return NULL */
-                    if (nb == priv->nbNodeCpus)
+                    if (nb == nbNodeCpus)
                         goto done;
 
                 }
@@ -1403,9 +1407,7 @@  xenUnifiedDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
         def = xenXMDomainGetXMLDesc(dom->conn, minidef);
     } else {
         char *cpus;
-        xenUnifiedLock(priv);
         cpus = xenDomainUsedCpus(dom);
-        xenUnifiedUnlock(priv);
         def = xenDaemonDomainGetXMLDesc(dom->conn, minidef, cpus);
         VIR_FREE(cpus);
     }