On 05.06.2019 12:09, 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_domain.c b/src/qemu/qemu_domain.c
> index 4d3a8868b2..f6b677c69e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7055,6 +7055,61 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
> };
>
>
> +/**
> + * qemuDomainObjListAdd:
> + * @driver: qemu driver
> + * @def: domain definition
> + * @oldDef: previous domain definition
> + * @live: whether @def is live definition
> + * @flags: an bitwise-OR of virDomainObjListAdd flags
> + *
> + * Add a domain onto the list of domain object and sets its
> + * definition. If @oldDef is not NULL and there was pre-existing
> + * definition it's returned in @oldDef.
> + *
> + * In addition to that, if definition of an existing domain is
> + * changed a MODIFY job is acquired prior to that.
> + *
> + * Returns: domain object pointer on success,
> + * NULL otherwise.
> + */
> +virDomainObjPtr
> +qemuDomainObjListAdd(virQEMUDriverPtr driver,
> + virDomainDefPtr def,
> + virDomainDefPtr *oldDef,
> + bool live,
> + unsigned int flags)
> +{
> + virDomainObjPtr vm = NULL;
> +
> + if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
> + return NULL;
> +
> + /* At this point, @vm is locked. If it doesn't have any
> + * definition set, then the call above just added it and
> + * there can't be anybody else using the object. It's safe to
> + * just set the definition without acquiring job. */
> + if (!vm->def) {
> + virDomainObjAssignDef(vm, def, live, oldDef);
> + VIR_RETURN_PTR(vm);
> + }
> +
> + /* Bad luck. Domain was pre-existing and this call is trying
> + * to update its definition. Modify job MUST be acquired. */
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
> + qemuDomainRemoveInactive(driver, vm);
Here we can remove transient domain.
Nikolay
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
