[libvirt] [PATCH] qemu: aquire job in qemuDomainDefineXMLFlags

[Michal Privoznik]

On 5/28/19 1:34 PM, Nikolay Shirokovskiy wrote:
> The function updates vm->newDef and frees previous pesistent definition
> which can be used by in different thread by some API that unlocks domain
> to communicate to qemu. So we have an access to freed memory in that
> thread. Let's acquire job condition when changing persistent xml.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
> 
> A bug example (some downstream version of libvirt, nevertheless):
> 
>      (gdb) bt
>      #0  0x00007f3bb77a0533 in virDomainDefHasVcpusOffline (def=def at entry=0x7f3b7802a830) at conf/domain_conf.c:1637
>      #1  0x00007f3bb77cb32f in virDomainCpuDefFormat (def=0x7f3b7802a830, buf=0x7f3ba7308940) at conf/domain_conf.c:27503
>      #2  virDomainDefFormatInternal (def=def at entry=0x7f3b7802a830, caps=0x7f3b70000ae0, flags=3, flags at entry=1,
>          buf=buf at entry=0x7f3ba7308940, xmlopt=xmlopt at entry=0x0) at conf/domain_conf.c:27850
>      #3  0x00007f3bb77ced46 in virDomainDefFormat (def=def at entry=0x7f3b7802a830, caps=<optimized out>,
>          flags=flags at entry=1) at conf/domain_conf.c:28564
>      #4  0x00007f3bb77d23f9 in virDomainSaveConfig (configDir=0x7f3b981227e0 "/etc/libvirt/qemu", caps=<optimized out>,
>          def=0x7f3b7802a830) at conf/domain_conf.c:28776
>      #5  0x00007f3ba086b617 in qemuDomainSetMemoryStatsPeriod (dom=<optimized out>, period=30, flags=<optimized out>)
>          at qemu/qemu_driver.c:4839
>      #6  0x00007f3bb7905613 in virDomainSetMemoryStatsPeriod (domain=domain at entry=0x7f3b74006200, period=30, flags=3)
>          at libvirt-domain.c:2087
> 
>      (gdb) f 5
>      #5  0x00007f3ba086b617 in qemuDomainSetMemoryStatsPeriod (dom=<optimized out>, period=30, flags=<optimized out>)
>          at qemu/qemu_driver.c:4839
>      4839            ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef);
> 
>      (gdb) p vm->def
>      $8 = (virDomainDefPtr) 0x7f3b8400c6f0
>      (gdb) p vm->newDef
>      $9 = (virDomainDefPtr) 0x7f3b7001de20
>      (gdb) p persistentDef
>      $10 = (virDomainDefPtr) 0x7f3b7802a830
> 
>   src/qemu/qemu_driver.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

I've managed to reproduce this. It's really simple. I've put a sleep() here (to give myself more time to run commands):

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 42b1ce2521..a5fe71e189 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -2405,6 +2405,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
 
         qemuDomainObjEnterMonitor(driver, vm);
         r = qemuMonitorSetMemoryStatsPeriod(priv->mon, def->memballoon, period);
+        sleep(60);
         if (qemuDomainObjExitMonitor(driver, vm) < 0)
             goto endjob;
         if (r < 0) {


And then:

1) virsh start $domain
2) virsh dommemstat --period 10 --config --live $domain
3) [from a different terminal]
   virsh edit $domain   # just add an empty line somewhere to enforce redefining the domain

4) Observe crashed daemon

> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 42b1ce2..683dcaa 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7626,6 +7626,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>       virCapsPtr caps = NULL;
>       unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
>                                  VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
> +    bool job = false;
>   
>       virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
>   
> @@ -7647,6 +7648,10 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>       if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
>           goto cleanup;
>   
> +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> +        goto cleanup;
> +    job = true;
> +
>       if (!(vm = virDomainObjListAdd(driver->domains, def,
>                                      driver->xmlopt,
>                                      0, &oldDef)))

This won't work really. @vm is NULL at the point of BeginJob() and the first thing that BeginJob() does is it dereferences @vm. At the same time, we can't just blindly put BeginJob() into virDomainObjListAdd() because that one is driver agnostic and BeginJob() is a qemu function. Maybe we can extend xmlopt for a new callback that if set is called just before a domain definition is changed. Then qemu driver would set this callback to BeginJob().

But this creates a problem because acquiring a job can take up to QEMU_JOB_WAIT_TIME seconds (currently 30) during which domain list is locked for write, i.e. not even 'virsh list' would be able to run meanwhile.

Michal