[libvirt] [PATCH] qemu: Remove hostdev entry when freeing the depending network entry
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); } -- 1.8.3.2
----- Original Message -----
From: "Peter Krempa" <pkrempa@redhat.com> To: libvir-list@redhat.com Cc: "Peter Krempa" <pkrempa@redhat.com> Sent: Tuesday, August 27, 2013 7:21:24 PM Subject: [libvirt] [PATCH] qemu: Remove hostdev entry when freeing the depending network entry
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)
s/>/>=/ The condition should fire also when the entry is present on index 0.
+ virDomainHostdevRemove(def, hostdev_index); + } + networkReleaseActualDevice(net); }
-- 1.8.3.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Peter
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. 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? 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?
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
participants (2)
-
Laine Stump -
Peter Krempa