[libvirt] [PATCH] qemu: Remove hostdev entry when freeing the depending network entry

[Peter Krempa]

On 08/28/13 07:17, Laine Stump wrote:
> On 08/27/2013 01:21 PM, Peter Krempa wrote:
>> When using a <interface type="network"> that points to a network with
>> hostdev forwarding mode a hostdev alias is created for the network. This
>> allias is inserted into the hostdev list, but is backed with a part of
>> the network object that it is connected to.
>>
>> When a VM is being stopped qemuProcessStop() calls
>> networkReleaseActualDevice() which eventually frees the memory for the
>> hostdev object. Afterwards when the domain definition is being freed by
>> virDomainDefFree() an invalid pointer is accessed by
>> virDomainHostdevDefFree() and may cause a crash of the daemon.
>>
>> This patch removes the entry in the hostdev list before freeing the
>> depending memory to avoid this issue.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1000973
>> ---
>>  src/qemu/qemu_process.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 128618b..2a69c8d 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -4241,6 +4241,9 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>>      def = vm->def;
>>      for (i = 0; i < def->nnets; i++) {
>>          virDomainNetDefPtr net = def->nets[i];
>> +        virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net);
>> +        int hostdev_index;
>> +
>>          if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) {
>>              ignore_value(virNetDevMacVLanDeleteWithVPortProfile(
>>                               net->ifname, &net->mac,
>> @@ -4259,6 +4262,11 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>>                                         virDomainNetGetActualBridgeName(net),
>>                                         net->ifname));
>>
>> +        if (hostdev) {
>> +            if ((hostdev_index = virDomainHostdevFind(def, hostdev, NULL)) > 0)
>> +                virDomainHostdevRemove(def, hostdev_index);
>> +        }
>> +
>>          networkReleaseActualDevice(net);
>>      }
>>
> 
> I would like to have as much of this type of teardown as possible
> directly in networkReleaseActualDevice rather than requiring the callers
> of that function to take care of it, but in this case
> networkReleaseActualDevice doesn't have (and shouldn't have) access to
> the full domain definition which it would need in order to get to the
> hostdevs array, so this is the way we have to do it.

As long as the network isn't added to the hostdevs in
networkAllocateActualDevice() it shouldn't be removed in
networkReleaseActualDevice().


> 
> ACK.
> 
> Two questions, though:
> 
> 1) Is this a problem that would exist on the older branches that are in
> RHEL6 and Fedora 18/19? Or did it only come about when the hostdev
> teardown was split into two parts so that we could take advantage of the
> "device is detached" event from qemu?

This problem is present in the RHEL6 tree, thus I don't think it's
related to the DEVICE_DEL event.

> 
> 2) Are the other places that networkReleaseActualDevice is called
> already getting their hostdev entry removed in some other way? Or do we
> need a similar patch in any other place?

Hmm, I was lazy when writing this patch and chose to look for
virDomainHostdevInsert() as an indicator to look for potential problems.
There was only one such place covered by this patch. Unfortunately in
qemu hotplug code, qemuDomainAttachHostDevice is used and I'll have to
inspect those code paths too and maybe they will also need treatment.

I'll push this patch shortly (with the tweak I mentioned in my previous
reply) and will look for other potential culprits.

Peter

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