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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list