[libvirt] [PATCH] qemu: Resolve agent deadlock

Monday, 26 October 2015

If we have a shutdown of a VM by a qemu agent while an agent EOF occurs
at relatively the same time, it's possible that a deadlock occurs depending
on what happens first.

Assume qemuProcessHandleAgentEOF runs first, with the vm->lock it clears
priv->agent, then clears the vm->lock.

After some agent call such as qemuDomainShutdownFlags will call
qemuDomainObjExitAgent in a worker thread with the agent lock. It
will then reference the priv->agent field:

    hasRefs = virObjectUnref(priv->agent);

in order to determine whether there are references. However, since
priv->agent is NULL, this returns false, so the subsquent:

    if (hasRefs)
        virObjectUnlock(priv->agent);

will not unlock the agent. Eventually the EOF thread will attempt
to call qemuAgentClose which will try to lock the agent, but cannot
since it's still locked.

This patch resolves this issue by keeping a copy of the priv->agent
address prior to releasing the vm->lock in qemuDomainObjEnterAgent.
Then rather than assuming priv->agent is valid, use the copy of the
agent pointer for the subsequent agent call and qemuDomainObjExitAgent.
Within qemuDomainObjExitAgent, use the agent copy in order to release
the agent lock, then get the vm->lock again and decide whether to
clear the priv->agent based on whether any references still exist.

Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt;
---
Recent upstream patch:

http://www.redhat.com/archives/libvir-list/2015-September/msg00971.html

attempted to undo commit id '1020a504' which appears to be the wrong
approach since it would still have the vm->lock during the qemuAgentClose.

 src/qemu/qemu_domain.c | 11 +++---
 src/qemu/qemu_domain.h |  4 +--
 src/qemu/qemu_driver.c | 90 +++++++++++++++++++++++++++++++++-----------------
 3 files changed, 67 insertions(+), 38 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 890d8ed..a908df8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1868,19 +1868,20 @@ qemuDomainObjEnterAgent(virDomainObjPtr obj)
  * Should be paired with an earlier qemuDomainObjEnterAgent() call
  */
 void
-qemuDomainObjExitAgent(virDomainObjPtr obj)
+qemuDomainObjExitAgent(virDomainObjPtr obj,
+                       qemuAgentPtr agent)
 {
     qemuDomainObjPrivatePtr priv = obj->privateData;
     bool hasRefs;
 
-    hasRefs = virObjectUnref(priv->agent);
+    hasRefs = virObjectUnref(agent);
 
     if (hasRefs)
-        virObjectUnlock(priv->agent);
+        virObjectUnlock(agent);
 
     virObjectLock(obj);
-    VIR_DEBUG("Exited agent (agent=%p vm=%p name=%s)",
-              priv->agent, obj, obj->def->name);
+    VIR_DEBUG("Exited agent (agent=%p vm=%p name=%s hasRefs=%d)",
+              agent, obj, obj->def->name, hasRefs);
 
     priv->agentStart = 0;
     if (!hasRefs)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 64cd7e1..def31a9 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -296,8 +296,8 @@ int qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
 
 void qemuDomainObjEnterAgent(virDomainObjPtr obj)
     ATTRIBUTE_NONNULL(1);
-void qemuDomainObjExitAgent(virDomainObjPtr obj)
-    ATTRIBUTE_NONNULL(1);
+void qemuDomainObjExitAgent(virDomainObjPtr obj, qemuAgentPtr agent)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 
 void qemuDomainObjEnterRemote(virDomainObjPtr obj)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a2cc002..b8c9ff7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1994,9 +1994,10 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int
flags)
     qemuDomainSetFakeReboot(driver, vm, isReboot);
 
     if (useAgent) {
+        qemuAgentPtr agent = priv->agent;
         qemuDomainObjEnterAgent(vm);
-        ret = qemuAgentShutdown(priv->agent, agentFlag);
-        qemuDomainObjExitAgent(vm);
+        ret = qemuAgentShutdown(agent, agentFlag);
+        qemuDomainObjExitAgent(vm, agent);
     }
 
     /* If we are not enforced to use just an agent, try ACPI
@@ -2087,9 +2088,10 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
     qemuDomainSetFakeReboot(driver, vm, isReboot);
 
     if (useAgent) {
+        qemuAgentPtr agent = priv->agent;
         qemuDomainObjEnterAgent(vm);
-        ret = qemuAgentShutdown(priv->agent, agentFlag);
-        qemuDomainObjExitAgent(vm);
+        ret = qemuAgentShutdown(agent, agentFlag);
+        qemuDomainObjExitAgent(vm, agent);
     }
 
     /* If we are not enforced to use just an agent, try ACPI
@@ -4864,6 +4866,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
     qemuAgentCPUInfoPtr cpuinfo = NULL;
     int ncpuinfo;
     qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
     size_t i;
     virCgroupPtr cgroup_temp = NULL;
     char *mem_mask = NULL;
@@ -4924,6 +4927,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
     }
 
     if (flags & VIR_DOMAIN_VCPU_GUEST) {
+
         if (!qemuDomainAgentAvailable(vm, true))
             goto endjob;
 
@@ -4935,19 +4939,20 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
             goto endjob;
         }
 
+        agent = priv->agent;
         qemuDomainObjEnterAgent(vm);
-        ncpuinfo = qemuAgentGetVCPUs(priv->agent, &cpuinfo);
-        qemuDomainObjExitAgent(vm);
+        ncpuinfo = qemuAgentGetVCPUs(agent, &cpuinfo);
+        qemuDomainObjExitAgent(vm, agent);
 
-        if (ncpuinfo < 0)
+        if (ncpuinfo < 0 || agent != priv->agent)
             goto endjob;
 
         if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0)
             goto endjob;
 
         qemuDomainObjEnterAgent(vm);
-        ret = qemuAgentSetVCPUs(priv->agent, cpuinfo, ncpuinfo);
-        qemuDomainObjExitAgent(vm);
+        ret = qemuAgentSetVCPUs(agent, cpuinfo, ncpuinfo);
+        qemuDomainObjExitAgent(vm, agent);
 
         if (ret < 0)
             goto endjob;
@@ -5515,6 +5520,8 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
         goto cleanup;
 
     if (flags & VIR_DOMAIN_VCPU_GUEST) {
+        qemuAgentPtr agent;
+
         if (!virDomainObjIsActive(vm)) {
             virReportError(VIR_ERR_INVALID_ARG, "%s",
                            _("vCPU count provided by the guest agent can only be
"
@@ -5534,9 +5541,10 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
             goto endjob;
         }
 
+        agent = priv->agent;
         qemuDomainObjEnterAgent(vm);
-        ncpuinfo = qemuAgentGetVCPUs(priv->agent, &cpuinfo);
-        qemuDomainObjExitAgent(vm);
+        ncpuinfo = qemuAgentGetVCPUs(agent, &cpuinfo);
+        qemuDomainObjExitAgent(vm, agent);
 
  endjob:
         qemuDomainObjEndJob(driver, vm);
@@ -13513,14 +13521,16 @@ qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
                            unsigned int nmountpoints)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
+    qemuAgentPtr agent;
     int frozen;
 
     if (!qemuDomainAgentAvailable(vm, true))
         return -1;
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
-    frozen = qemuAgentFSFreeze(priv->agent, mountpoints, nmountpoints);
-    qemuDomainObjExitAgent(vm);
+    frozen = qemuAgentFSFreeze(agent, mountpoints, nmountpoints);
+    qemuDomainObjExitAgent(vm, agent);
     return frozen < 0 ? -2 : frozen;
 }
 
@@ -13532,19 +13542,21 @@ qemuDomainSnapshotFSThaw(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
                          bool report)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
+    qemuAgentPtr agent;
     int thawed;
     virErrorPtr err = NULL;
 
     if (!qemuDomainAgentAvailable(vm, report))
         return -1;
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
     if (!report)
         err = virSaveLastError();
-    thawed = qemuAgentFSThaw(priv->agent);
+    thawed = qemuAgentFSThaw(agent);
     if (!report)
         virSetError(err);
-    qemuDomainObjExitAgent(vm);
+    qemuDomainObjExitAgent(vm, agent);
 
     virFreeError(err);
 
@@ -18146,6 +18158,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
     virDomainObjPtr vm;
     int ret = -1;
 
@@ -18218,9 +18231,10 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
         goto endjob;
     }
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentSuspend(priv->agent, target);
-    qemuDomainObjExitAgent(vm);
+    ret = qemuAgentSuspend(agent, target);
+    qemuDomainObjExitAgent(vm, agent);
 
  endjob:
     qemuDomainObjEndJob(driver, vm);
@@ -18309,6 +18323,7 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
     int ret = -1;
     char *result = NULL;
     qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
 
     virCheckFlags(0, NULL);
 
@@ -18338,9 +18353,10 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
         goto endjob;
     }
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentArbitraryCommand(priv->agent, cmd, &result, timeout);
-    qemuDomainObjExitAgent(vm);
+    ret = qemuAgentArbitraryCommand(agent, cmd, &result, timeout);
+    qemuDomainObjExitAgent(vm, agent);
     if (ret < 0)
         VIR_FREE(result);
 
@@ -18411,6 +18427,7 @@ qemuDomainFSTrim(virDomainPtr dom,
     virDomainObjPtr vm;
     int ret = -1;
     qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
 
     virCheckFlags(0, -1);
 
@@ -18447,9 +18464,10 @@ qemuDomainFSTrim(virDomainPtr dom,
         goto endjob;
     }
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentFSTrim(priv->agent, minimum);
-    qemuDomainObjExitAgent(vm);
+    ret = qemuAgentFSTrim(agent, minimum);
+    qemuDomainObjExitAgent(vm, agent);
 
  endjob:
     qemuDomainObjEndJob(driver, vm);
@@ -18600,6 +18618,7 @@ qemuDomainGetTime(virDomainPtr dom,
     virQEMUDriverPtr driver = dom->conn->privateData;
     virDomainObjPtr vm = NULL;
     qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
     int ret = -1;
     int rv;
 
@@ -18625,9 +18644,10 @@ qemuDomainGetTime(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
-    rv = qemuAgentGetTime(priv->agent, seconds, nseconds);
-    qemuDomainObjExitAgent(vm);
+    rv = qemuAgentGetTime(agent, seconds, nseconds);
+    qemuDomainObjExitAgent(vm, agent);
 
     if (rv < 0)
         goto endjob;
@@ -18650,6 +18670,7 @@ qemuDomainSetTime(virDomainPtr dom,
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
     virDomainObjPtr vm;
     bool rtcSync = flags & VIR_DOMAIN_TIME_SYNC;
     int ret = -1;
@@ -18689,9 +18710,10 @@ qemuDomainSetTime(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
-    rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync);
-    qemuDomainObjExitAgent(vm);
+    rv = qemuAgentSetTime(agent, seconds, nseconds, rtcSync);
+    qemuDomainObjExitAgent(vm, agent);
 
     if (rv < 0)
         goto endjob;
@@ -19647,6 +19669,7 @@ qemuDomainGetFSInfo(virDomainPtr dom,
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
     virDomainObjPtr vm;
     int ret = -1;
 
@@ -19672,9 +19695,10 @@ qemuDomainGetFSInfo(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentGetFSInfo(priv->agent, info, vm->def);
-    qemuDomainObjExitAgent(vm);
+    ret = qemuAgentGetFSInfo(agent, info, vm->def);
+    qemuDomainObjExitAgent(vm, agent);
 
  endjob:
     qemuDomainObjEndJob(driver, vm);
@@ -19692,6 +19716,7 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     qemuDomainObjPrivatePtr priv = NULL;
+    qemuAgentPtr agent;
     virDomainObjPtr vm = NULL;
     int ret = -1;
 
@@ -19723,9 +19748,10 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
         if (!qemuDomainAgentAvailable(vm, true))
             goto endjob;
 
+        agent = priv->agent;
         qemuDomainObjEnterAgent(vm);
-        ret = qemuAgentGetInterfaces(priv->agent, ifaces);
-        qemuDomainObjExitAgent(vm);
+        ret = qemuAgentGetInterfaces(agent, ifaces);
+        qemuDomainObjExitAgent(vm, agent);
 
     endjob:
         qemuDomainObjEndJob(driver, vm);
@@ -19850,6 +19876,7 @@ qemuDomainSetUserPassword(virDomainPtr dom,
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
     virDomainObjPtr vm;
     int ret = -1;
     int rv;
@@ -19876,10 +19903,11 @@ qemuDomainSetUserPassword(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
+    agent = priv->agent;
     qemuDomainObjEnterAgent(vm);
-    rv = qemuAgentSetUserPassword(priv->agent, user, password,
+    rv = qemuAgentSetUserPassword(agent, user, password,
                                   flags & VIR_DOMAIN_PASSWORD_ENCRYPTED);
-    qemuDomainObjExitAgent(vm);
+    qemuDomainObjExitAgent(vm, agent);
 
     if (rv < 0)
         goto endjob;
-- 
2.1.0


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH] qemu: Resolve agent deadlock