On 08/07/14 18:36, Eric Blake wrote:
On 08/07/2014 01:58 AM, Peter Krempa wrote:
On 08/06/14 23:12, Eric Blake wrote:
Commit febf84c2 tried to delay in-memory modification of the actual domain disk structure until after the qemu event was received. However, I missed that the code for block pivot had been temporarily setting disk->src = disk->mirror prior to the qemu command, in order to label the backing chain of a reused external blockcopy disk; and calls into qemu while still in that state before finally undoing things at the cleanup label. Since the qemu event handler then does: virStorageSourceFree(disk->src); disk->src = disk->mirror; we have the sad race that a fast enough qemu event can cause a leak of the original disk->src, as well as a use-after-free of the disk->mirror contents, bad enough to crash libvirtd in some of my test runs, even though the common case of the qemu event being much later won't trip the race.
- if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) - goto cleanup; + if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) + goto cleanup;
If we go to cleanup here...
- if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW && - (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, disk) < 0 || - qemuSetupDiskCgroup(vm, disk) < 0 || - virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, disk) < 0)) - goto cleanup; + if (disk->mirror->format && + disk->mirror->format != VIR_STORAGE_FILE_RAW && + (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, + disk) < 0 || + qemuSetupDiskCgroup(vm, disk) < 0 || + virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, + disk) < 0)) + goto cleanup; + + disk->src = oldsrc; + oldsrc = NULL;
...then we miss the restore of disk->src here...
+ }
/* Attempt the pivot. Record the attempt now, to prevent duplicate * attempts; but the actual disk change will be made when emitting
In the cleanup section there's the original place where oldsrc was returned back to disk->src. That would now be dead code.
...so the cleanup label is not dead code.
ACK with that part removed.
Is the patch okay as-is, with my explanation?
Ah right :) Peter