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@xxxxxxxxxxxxx> ---
>> 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...
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list