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@xxxxxxxxxx>
> ---
> 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?
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto endjob;
> if (r < 0) {
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
