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

[Nikolay Shirokovskiy]

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 at redhat.com>
> ---
>  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