[libvirt] [PATCH] network: fix crash when starting a network with no <pf> element
Martin Kletzander pointed out in email that my commit 2a193f64 introduced a crash in networkCreateInterfacePool() during startup of any network that doesn't have a <pf> subelement of its <forward> element. He also supplied a patch. http://www.redhat.com/archives/libvir-list/2014-August/msg00655.html I expanded on that patch by cleaning uyp now-extraneous checks in the callers of networkCreateInterfacePool(). Fortunately the offending patch hasn't been in any release, and hasn't been (to my knowledge) backported to any other branch. --- src/network/bridge_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1ba4c3d..fc4c73d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2196,6 +2196,9 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) int ret = -1; size_t i; + if (!netdef->forward.npfs || netdef->forward.nifs > 0) + return 0; + if ((virNetDevGetVirtualFunctions(netdef->forward.pfs->dev, &vfNames, &virtFns, &numVirtFns)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2204,7 +2207,6 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) goto cleanup; } - netdef->forward.nifs = 0; if (VIR_ALLOC_N(netdef->forward.ifs, numVirtFns) < 0) goto cleanup; @@ -3843,10 +3845,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, virDomainHostdevSubsysPCIBackendType backend; iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_HOSTDEV; - if (netdef->forward.npfs > 0 && netdef->forward.nifs <= 0 && - networkCreateInterfacePool(netdef) < 0) { + if (networkCreateInterfacePool(netdef) < 0) goto error; - } /* pick first dev with 0 connections */ for (i = 0; i < netdef->forward.nifs; i++) { @@ -3978,10 +3978,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, } else { /* pick an interface from the pool */ - if (netdef->forward.npfs > 0 && netdef->forward.nifs == 0 && - networkCreateInterfacePool(netdef) < 0) { + if (networkCreateInterfacePool(netdef) < 0) goto error; - } /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both * require exclusive access to a device, so current @@ -4149,10 +4147,9 @@ networkNotifyActualDevice(virDomainDefPtr dom, goto success; } - if (netdef->forward.npfs > 0 && netdef->forward.nifs == 0 && - networkCreateInterfacePool(netdef) < 0) { + if (networkCreateInterfacePool(netdef) < 0) goto error; - } + if (netdef->forward.nifs == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' uses a direct or hostdev mode, " -- 1.9.3
On Thu, Aug 14, 2014 at 12:40:07PM -0400, Laine Stump wrote:
Martin Kletzander pointed out in email that my commit 2a193f64 introduced a crash in networkCreateInterfacePool() during startup of any network that doesn't have a <pf> subelement of its <forward> element. He also supplied a patch.
http://www.redhat.com/archives/libvir-list/2014-August/msg00655.html
I expanded on that patch by cleaning uyp now-extraneous checks in the callers of
s/uyp/up/
networkCreateInterfacePool().
Fortunately the offending patch hasn't been in any release, and hasn't been (to my knowledge) backported to any other branch. --- src/network/bridge_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1ba4c3d..fc4c73d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2196,6 +2196,9 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) int ret = -1; size_t i;
+ if (!netdef->forward.npfs || netdef->forward.nifs > 0)
I'd match these to use both !var (or var != 0), but other than that the patch is fine, ACK with that fixed. Martin
On 08/14/2014 05:10 PM, Martin Kletzander wrote:
On Thu, Aug 14, 2014 at 12:40:07PM -0400, Laine Stump wrote:
src/network/bridge_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1ba4c3d..fc4c73d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2196,6 +2196,9 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) int ret = -1; size_t i;
+ if (!netdef->forward.npfs || netdef->forward.nifs > 0)
I'd match these to use both !var (or var != 0), but other than that the patch is fine, ACK with that fixed.
I decided to go with == 0 and > 0. They are closest to what is happening in my mind when I think about the condition, so hopefully will be the easiest for others to understand. Thanks!
On Fri, Aug 15, 2014 at 02:50:30AM -0400, Laine Stump wrote:
On 08/14/2014 05:10 PM, Martin Kletzander wrote:
On Thu, Aug 14, 2014 at 12:40:07PM -0400, Laine Stump wrote:
src/network/bridge_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1ba4c3d..fc4c73d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2196,6 +2196,9 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) int ret = -1; size_t i;
+ if (!netdef->forward.npfs || netdef->forward.nifs > 0)
I'd match these to use both !var (or var != 0), but other than that the patch is fine, ACK with that fixed.
I decided to go with == 0 and > 0. They are closest to what is happening in my mind when I think about the condition, so hopefully will be the easiest for others to understand.
Thanks!
Thank you for such quick response. It's still weird though, because it didn't segfault every start and when it segfaulted, it wasn't on this line, but in some malloc(), which means we must have screwed up something pretty bad. But valgrind suggested there is an invalid read in here, so let's hope this fixes it :) Martin
participants (2)
-
Laine Stump -
Martin Kletzander