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

[Peter Krempa]

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 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_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) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190605/6c84d83b/attachment-0001.sig>