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

[Michal Privoznik]

On 6/5/19 11:32 AM, Peter Krempa wrote:
> 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?

Oh, right O:-)

Consider removed.

Michal