Re: [libvirt] [PATCH v2 06/11] qemu: Grab modify job for changing domain XML

On Wed, Jun 05, 2019 at 11:09:14 +0200, Michal Privoznik wrote: > Changing domain definition must be guarded with acquiring modify > job. The problem is if there is a thread executing say > qemuDomainSetMemoryStatsPeriod() which is an API that acquires > modify job and then possibly unlock the domain object and locks > it again. However, becasue virDomainObjAssignDef() does not take > a job (only object lock) it may have changed the domain > definition while the other thread unlocked the domain object in > order to talk on the monitor. For instance: > > Thread1: > 1) lookup domain object and lock it > 2) acquire job > 3) get pointers to live and persistent defs > 4) unlock the domain object > 5) talk to qemu on the monitor > > Thread2 (Execute qemuDomainDefineXMLFlags): > 1) lookup domain object and lock it > 2) virDomainObjAssignDef() which overwrites persistent def and > frees the old one > 3) unlock domain object > > Thread1: > 6) lock the domain object again > 7) try to access the persistent def via pointer stored in 3) > > Now, this will crash because the pointer from step 3) points to a > memory that was freed. > > However, if Thread2 waited and acquired modify job (just like > Thread1) then these two would serialize and no harm would be > done. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_domain.c | 55 +++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_domain.h | 6 +++++ > src/qemu/qemu_driver.c | 27 ++++++------------- > src/qemu/qemu_migration.c | 5 +--- > 4 files changed, 70 insertions(+), 23 deletions(-) [...] > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 8bbac339e0..fa93a686b7 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -1701,12 +1701,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, > if (virDomainCreateXMLEnsureACL(conn, def) < 0) > goto cleanup; > > - if (!(vm = virDomainObjListAdd(driver->domains, def, > - driver->xmlopt, > - VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE))) > + if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true, > + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE))) > goto cleanup; > - > - virDomainObjAssignDef(vm, def, true, NULL); > def = NULL; > > if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START, > @@ -2405,6 +2402,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, > > qemuDomainObjEnterMonitor(driver, vm); > r = qemuMonitorSetMemoryStatsPeriod(priv->mon, def->memballoon, period); > + sleep(60); Debugging leftovers?