Re: [libvirt] [PATCH] qemu: fix libvirtd crash when querying halted cpus info

On 15.11.2016 13:19, Peter Krempa wrote: > On Wed, Nov 02, 2016 at 18:56:49 +0300, Maxim Nestratov wrote: >> It was introduced by commit 7a51d9ebb, which started to use >> monitor commands without job acquiring, which is unsafe and >> leads to simultaneous access to vm->mon structure by different >> threads. >> >> Crash backtrace is the following (shortened): >> >> Program received signal SIGSEGV, Segmentation fault. >> qemuMonitorSend (mon=mon@entry=0x7f4ef4000d20, >> msg=msg@entry=0x7f4f18e78640) at qemu/qemu_monitor.c:1011 1011 >> while (!mon->msg->finished) { >> >> 0 qemuMonitorSend () at qemu/qemu_monitor.c:1011 1 >> 0x00007f691abdc720 in qemuMonitorJSONCommandWithFd () at >> qemu/qemu_monitor_json.c:298 2 0x00007f691abde64a in >> qemuMonitorJSONCommand at qemu/qemu_monitor_json.c:328 3 >> qemuMonitorJSONQueryCPUs at qemu/qemu_monitor_json.c:1408 4 >> 0x00007f691abcaebd in qemuMonitorGetCPUInfo g@entry=false) at >> qemu/qemu_monitor.c:1931 5 0x00007f691ab96863 in >> qemuDomainRefreshVcpuHalted at qemu/qemu_domain.c:6309 6 >> 0x00007f691ac0af99 in qemuDomainGetStatsVcpu at >> qemu/qemu_driver.c:18945 7 0x00007f691abef921 in >> qemuDomainGetStats at qemu/qemu_driver.c:19469 8 >> qemuConnectGetAllDomainStats at qemu/qemu_driver.c:19559 9 >> 0x00007f693382e806 in virConnectGetAllDomainStats at >> libvirt-domain.c:11546 10 0x00007f6934470c40 in >> remoteDispatchConnectGetAllDomainStats at remote.c:6267 >> >> (gdb) p mon->msg $1 = (qemuMonitorMessagePtr) 0x0 >> >> This change fixes it by calling qemuDomainRefreshVcpuHalted only >> when job is acquired. >> >> Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com&gt; --- >> src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, >> 13 insertions(+), 7 deletions(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index dd1907b..43a546d 100644 --- a/src/qemu/qemu_driver.c +++ >> b/src/qemu/qemu_driver.c @@ -18915,7 +18915,7 @@ >> qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, virDomainObjPtr >> dom, virDomainStatsRecordPtr record, int *maxparams, - >> unsigned int privflags ATTRIBUTE_UNUSED) + >> unsigned int privflags) { size_t i; int ret = -1; @@ -18939,14 >> +18939,20 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, >> return -1; >> >> if (VIR_ALLOC_N(cpuinfo, virDomainDefGetVcpus(dom->def)) < 0 || - >> VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0) - >> goto cleanup; - - if (qemuDomainRefreshVcpuHalted(driver, >> dom, - QEMU_ASYNC_JOB_NONE) == >> 0 && + VIR_ALLOC_N(cpuwait, >> virDomainDefGetVcpus(dom->def)) < 0 || VIR_ALLOC_N(cpuhalted, >> virDomainDefGetVcpus(dom->def)) < 0) goto cleanup; >> >> + if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) { + > Spurious empty line. > >> + if (qemuDomainRefreshVcpuHalted(driver, dom, + >> QEMU_ASYNC_JOB_NONE) < 0) { + /* it's ok to be silent >> and go ahead, because halted vcpu info + * wasn't >> here from the beginning */ + virResetLastError(); + >> } + } + > Not visible in this context: The output of the parameter needs to > be suppressed if the parameter can't be requested. > > Peter > right, the VIR_ALLOC_N(cpuhalted...) should be conditional on the success of qemuDomainRefreshVcpuHalted, as it was originally...