11:25 a.m.
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(a)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
12:46 p.m.
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(a)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
12:59 p.m.
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(a)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
8:13 a.m.
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(a)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
9:28 a.m.
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
4327
days inactive
4328
days old
4 comments
3 participants
participants (3)
-
Eric Blake -
Jiri Denemark -
Viktor Mihajlovski