[libvirt] [PATCHv2 03/14] Fix vmdef usage after domain crash in monitor on device removal

[Ján Tomko]

On 01/12/2015 10:44 PM, John Ferlan wrote:
> 
> 
> On 01/07/2015 10:42 AM, Ján Tomko wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1161024
>>
>> In the device type-specific functions, exit early
>> if the domain has disappeared, because the cleanup
>> should have been done by qemuProcessStop.
>>
>> Check the return value in processDeviceDeletedEvent
>> and qemuProcessUpdateDevices.
>> ---
>>  src/qemu/qemu_driver.c  |  3 ++-
>>  src/qemu/qemu_hotplug.c | 32 ++++++++++++++++++++------------
>>  src/qemu/qemu_hotplug.h |  6 +++---
>>  src/qemu/qemu_process.c |  6 ++++--
>>  4 files changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index cdf4173..f7c9219 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4044,7 +4044,8 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
>>      if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0)
>>          goto endjob;
>>  
>> -    qemuDomainRemoveDevice(driver, vm, &dev);
>> +    if (qemuDomainRemoveDevice(driver, vm, &dev) < 0)
>> +        goto endjob;
>>  
>>      if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>>          VIR_WARN("unable to save domain status after removing device %s",
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 1714341..ce05f80 100644
> 
> The following generally applies to patches 3-5...
> 
> For hotplug, the ExitMonitor failures are done prior to auditing or
> sending events. Although not all the code handles failures and auditing
> quite the same - I would think we still want to Audit the qemuMonitor*
> calls regardless of whether the process dies or not. The audit code uses
> vm->def->{uuid|name|virtType} to format the message it generates, so is
> that "safe" according to what happens on the ExitMonitor failure?
> 
> One reason this springs to mind is some of the code will Audit on the
> qemuMonitor* call success/failure and I would think perhaps one of the
> failures is that the vm died and having that Audit trail could be a good
> thing. Beyond that knowing that the qemuMonitor* call passed (because
> Audit told us so), but then finding the vm crashed (because ExitMonitor
> told us so) could narrow some other bizarre window.
> 
> So while perhaps slightly outside the scope of these changes - what
> affect does exiting prior to Audit really have... and yes, event is
> different can of worms.
> 
> As I go deeper into the patches I see the HotplugVcpus will call
> virDomainAuditVcpu even after the ignored ExitMonitor, so it seems safe...
> 

For HotplugVcpus, we only pass the number of vcpus to the audit function. For
hotplug, the Attach* function is the one owning the pointer to the device
(except for the chardev attach function), so we can safely audit that.

The skipped audits are those using pointers that are in the vm->def and could
be freed by qemuProcessStop in another thread in the meantime. I thought the
domain crash audit in there would be enough.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150113/f16b9dbb/attachment-0001.sig>