[libvirt] [PATCH] qemu: Double mutex unlock in qemuDomainModifyDeviceFlags
The driver mutex was unlocked in qemuDomainModifyDeviceFlags before entering qemuDomainObjBeginJobWithDriver where it will be unlocked once more leaving it in an undefined state. The result was that two threads were simultaneously looking up the domain hash table during multiple parallel device attach/detach operations. Luckily this triggered a virHashIterationError. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c39864..c28c223 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6443,7 +6443,6 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); - qemuDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); -- 1.7.9.5
On Thu, Jan 17, 2013 at 18:25:28 +0100, Viktor Mihajlovski wrote:
The driver mutex was unlocked in qemuDomainModifyDeviceFlags before entering qemuDomainObjBeginJobWithDriver where it will be unlocked once more leaving it in an undefined state. The result was that two threads were simultaneously looking up the domain hash table during multiple parallel device attach/detach operations. Luckily this triggered a virHashIterationError.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c39864..c28c223 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6443,7 +6443,6 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); - qemuDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr);
ACK. The function relies on driver being locked and unlocks it in cleanup phase. Jirka
On 01/17/2013 11:46 AM, Jiri Denemark wrote:
On Thu, Jan 17, 2013 at 18:25:28 +0100, Viktor Mihajlovski wrote:
The driver mutex was unlocked in qemuDomainModifyDeviceFlags before entering qemuDomainObjBeginJobWithDriver where it will be unlocked once more leaving it in an undefined state. The result was that two threads were simultaneously looking up the domain hash table during multiple parallel device attach/detach operations. Luckily this triggered a virHashIterationError.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> ---
ACK. The function relies on driver being locked and unlocks it in cleanup phase.
Pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/17/2013 10:25 AM, Viktor Mihajlovski wrote:
The driver mutex was unlocked in qemuDomainModifyDeviceFlags before entering qemuDomainObjBeginJobWithDriver where it will be unlocked once more leaving it in an undefined state. The result was that two threads were simultaneously looking up the domain hash table during multiple parallel device attach/detach operations. Luckily this triggered a virHashIterationError.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 1 - 1 file changed, 1 deletion(-)
I tracked this down to commit 8c5d2ba; it looks like Michal encountered a case of git botching a rebase when forward-porting a patch. If you'll look at that commit, we removed a qemuDriverUnlock() from one function (qemuDomainSendKey), added it to another (qemuDomainModifyDeviceFlags), neither in a pair; and neither matches the commit message mention of DomainHasManagedSaveImage. You only repaired one piece of the damage, but missed the other damage, and we still haven't fixed the intended function. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/18/2013 03:13 PM, Eric Blake wrote:
I tracked this down to commit 8c5d2ba; it looks like Michal encountered a case of git botching a rebase when forward-porting a patch. If you'll look at that commit, we removed a qemuDriverUnlock() from one function (qemuDomainSendKey), added it to another (qemuDomainModifyDeviceFlags), neither in a pair; and neither matches the commit message mention of DomainHasManagedSaveImage. You only repaired one piece of the damage, but missed the other damage, and we still haven't fixed the intended function.
I was trying to make out what went wrong with that commit, but so far it looked good to me. Specifically, qemuDomainMonitorCommand looked sane and I wasn't seeing anything lock related in qemuDomainHasManagedSaveImage. Looking again, it seems that there's now a superfluous DriverUnlock in the qemuDomainManagedSave (log message?) cleanup section which actually belongs to qemuDomainSendKey. I will send out a patch for that soon. Thanks for pointing out the asymmetry. P.S: I was selectively replacing the mutexes for the driver with error checking ones to find the place of mutex corruption. Is there any interest in making this configurable for test purposes? If so (and time allows) I could prepare something for review. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (3)
-
Eric Blake -
Jiri Denemark -
Viktor Mihajlovski