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

[Maxim Nestratov]

15-Nov-16 15:38, Viktor Mihajlovski пишет:

> 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 at entry=0x7f4ef4000d20,
>>> msg=msg at 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 at 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 at virtuozzo.com> ---
>>> 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...
>

Agree. Sent a new version.

Maxim