[libvirt] [PATCHv2 0/2] Fix crash of libvirtd with <interaface type=network and hostdev forwarding
Version 2 contains a fix of more callers in qemu and a whitespace cleanup. Peter Krempa (2): qemu: Remove hostdev entry when freeing the depending network entry qemu_hotplug: Fix whitespace around addition in argument src/conf/domain_conf.c | 26 ++++++++++++++++++-------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 9 +++++++-- src/qemu/qemu_process.c | 2 ++ 5 files changed, 29 insertions(+), 10 deletions(-) -- 1.8.3.2
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/conf/domain_conf.c | 26 ++++++++++++++++++-------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 5 +++++ src/qemu/qemu_process.c | 2 ++ 5 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9b1387d..f8fbf79 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9965,26 +9965,36 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) return matchidx; } -virDomainNetDefPtr -virDomainNetRemove(virDomainDefPtr def, size_t i) -{ - virDomainNetDefPtr net = def->nets[i]; +void +virDomainNetRemoveHostdev(virDomainDefPtr def, + virDomainNetDefPtr net) +{ if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* hostdev net devices are normally also be in the hostdevs * array, but might have already been removed by the time we * get here. */ virDomainHostdevDefPtr hostdev = &net->data.hostdev.def; - size_t h; + size_t i; - for (h = 0; h < def->nhostdevs; h++) { - if (def->hostdevs[h] == hostdev) { - virDomainHostdevRemove(def, h); + for (i = 0; i < def->nhostdevs; i++) { + if (def->hostdevs[i] == hostdev) { + virDomainHostdevRemove(def, i); break; } } } +} + + +virDomainNetDefPtr +virDomainNetRemove(virDomainDefPtr def, size_t i) +{ + virDomainNetDefPtr net = def->nets[i]; + + virDomainNetRemoveHostdev(def, net); + if (def->nnets > 1) { memmove(def->nets + i, def->nets + i + 1, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0ac576a..56739b7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2371,6 +2371,7 @@ int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net); virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char *device); int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net); virDomainNetDefPtr virDomainNetRemove(virDomainDefPtr def, size_t i); +void virDomainNetRemoveHostdev(virDomainDefPtr def, virDomainNetDefPtr net); int virDomainHostdevInsert(virDomainDefPtr def, virDomainHostdevDefPtr hostdev); virDomainHostdevDefPtr diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d4006ae..35f0f1b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -303,6 +303,7 @@ virDomainNetGetActualVirtPortProfile; virDomainNetGetActualVlan; virDomainNetInsert; virDomainNetRemove; +virDomainNetRemoveHostdev; virDomainNetTypeToString; virDomainNostateReasonTypeFromString; virDomainNostateReasonTypeToString; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5943816..a7256f3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1066,6 +1066,8 @@ cleanup: virDomainNetGetActualBridgeName(net), net->ifname)); } + virDomainNetRemoveHostdev(vm->def, net); + networkReleaseActualDevice(net); } @@ -2107,6 +2109,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, /* the changes above warrant replacing olddev with newdev in * the domain's nets list. */ + + /* this function doesn't work with HOSTDEV networks yet, thus + * no need to change the pointer in the hostdev structure */ networkReleaseActualDevice(olddev); virDomainNetDefFree(olddev); /* move newdev into the nets list, and NULL it out from the diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 128618b..dfe8142 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4259,6 +4259,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainNetGetActualBridgeName(net), net->ifname)); + /* kick the device out of the hostdev list too */ + virDomainNetRemoveHostdev(def, net); networkReleaseActualDevice(net); } -- 1.8.3.2
--- src/qemu/qemu_hotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a7256f3..9c655ff 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -814,7 +814,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, size_t i; /* preallocate new slot for device */ - if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) + if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0) goto cleanup; /* If appropriate, grab a physical device from the configured @@ -1136,7 +1136,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, char *configfd_name = NULL; bool releaseaddr = false; - if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) return -1; if (qemuPrepareHostdevPCIDevices(driver, vm->def->name, vm->def->uuid, -- 1.8.3.2
On 28.08.2013 15:14, Peter Krempa wrote:
Version 2 contains a fix of more callers in qemu and a whitespace cleanup.
Peter Krempa (2): qemu: Remove hostdev entry when freeing the depending network entry qemu_hotplug: Fix whitespace around addition in argument
src/conf/domain_conf.c | 26 ++++++++++++++++++-------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 9 +++++++-- src/qemu/qemu_process.c | 2 ++ 5 files changed, 29 insertions(+), 10 deletions(-)
ACK series. Although, the patch 2/2 - there is more of cases like that in our code base: $ git grep "[^ +]+[^ +]" src/ | wc -l 434 (yes, I know there are plenty of false positives) Michal
On 08/28/13 16:43, Michal Privoznik wrote:
On 28.08.2013 15:14, Peter Krempa wrote:
Version 2 contains a fix of more callers in qemu and a whitespace cleanup.
Peter Krempa (2): qemu: Remove hostdev entry when freeing the depending network entry qemu_hotplug: Fix whitespace around addition in argument
src/conf/domain_conf.c | 26 ++++++++++++++++++-------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 9 +++++++-- src/qemu/qemu_process.c | 2 ++ 5 files changed, 29 insertions(+), 10 deletions(-)
ACK series. Although, the patch 2/2 - there is more of cases like that in our code base:
$ git grep "[^ +]+[^ +]" src/ | wc -l 434
(yes, I know there are plenty of false positives)
Michal
Pushed; Thanks. Peter
participants (2)
-
Michal Privoznik -
Peter Krempa