1:59 p.m.
Each of these has an associated bugzilla record; 1/2 against RHEL6 and
2/2 against RHEL7, but both bugs exist in all versions of libvirt that
support the <pf dev='xyz'/> element. 1/2 is more serious as it can
lead to a crash, while 2/2 just changes the timing of error
reporting.the discovery of bad configurations.
Laine Stump (2):
network: make networkCreateInterfacePool more robust
network: populate interface pool immediately when network is started
src/network/bridge_driver.c | 179 ++++++++++++++++++++++++++------------------
1 file changed, 107 insertions(+), 72 deletions(-)
--
1.9.3
1:59 p.m.
New subject: [libvirt] [PATCH 1/2] network: make networkCreateInterfacePool more robust
networkCreateInterfacePool was a bit loose in its error cleanup, which
could result in a network definition with interfaces in the pool that
were NULL. This would in turn lead to a libvirtd crash when a guest
tried to attach an interface using the network with that pool.
In particular this would happen when creating a pool to be used for
macvtap connections. macvtap needs the netdev name of the virtual
function in order to use it, and each VF only has a netdev name if it
is currently bound to a network driver. If one of the VFs of a PF
happened to be bound to the pci-stub or vfio-pci driver (indicating
it's already in use for PCI passthrough), or no driver at all, it
would have no name. In this case networkCreateInterfacePool would
return an error, but would leave the netdef->forward.nifs set to the
total number of VFs in the PF. The interface attach that triggered
calling of networkCreateInterfacePool (it uses a "lazy fill" strategy)
would simply fail, but the very next attempt to attach an interface
using the same network pool would result in a crash.
This patch refactors networkCreateInterfacePool to bring it more in
line with current coding practices (label name, use of a switch with
no default case) as well as providing the following two changes to
behavior:
1) If a VF with no netdev name is encountered, just log a warning and
continue; only fail if exactly 0 devices are found to put in the pool.
2) If the function fails, clean up any partial interface pool and set
netdef->forward.nifs to 0.
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1111455
---
src/network/bridge_driver.c | 113 ++++++++++++++++++++++++++++----------------
1 file changed, 73 insertions(+), 40 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3e4868a..918de29 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3608,65 +3608,98 @@ int networkRegister(void)
static int
networkCreateInterfacePool(virNetworkDefPtr netdef)
{
- size_t num_virt_fns = 0;
- char **vfname = NULL;
- virPCIDeviceAddressPtr *virt_fns;
+ size_t numVirtFns = 0;
+ char **vfNames = NULL;
+ virPCIDeviceAddressPtr *virtFns;
+
int ret = -1;
size_t i;
if ((virNetDevGetVirtualFunctions(netdef->forward.pfs->dev,
- &vfname, &virt_fns, &num_virt_fns))
< 0) {
+ &vfNames, &virtFns, &numVirtFns)) <
0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not get Virtual functions on %s"),
netdef->forward.pfs->dev);
- goto finish;
- }
-
- if (num_virt_fns == 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("No Vf's present on SRIOV PF %s"),
- netdef->forward.pfs->dev);
- goto finish;
+ goto cleanup;
}
- if (VIR_ALLOC_N(netdef->forward.ifs, num_virt_fns) < 0)
- goto finish;
+ netdef->forward.nifs = 0;
+ if (VIR_ALLOC_N(netdef->forward.ifs, numVirtFns) < 0)
+ goto cleanup;
- netdef->forward.nifs = num_virt_fns;
+ for (i = 0; i < numVirtFns; i++) {
+ virPCIDeviceAddressPtr thisVirtFn = virtFns[i];
+ const char *thisName = vfNames[i];
+ virNetworkForwardIfDefPtr thisIf
+ = &netdef->forward.ifs[netdef->forward.nifs];
- for (i = 0; i < netdef->forward.nifs; i++) {
- if ((netdef->forward.type == VIR_NETWORK_FORWARD_BRIDGE) ||
- (netdef->forward.type == VIR_NETWORK_FORWARD_PRIVATE) ||
- (netdef->forward.type == VIR_NETWORK_FORWARD_VEPA) ||
- (netdef->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH)) {
- netdef->forward.ifs[i].type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV;
- if (vfname[i]) {
- if (VIR_STRDUP(netdef->forward.ifs[i].device.dev, vfname[i]) < 0)
- goto finish;
+ switch (netdef->forward.type) {
+ case VIR_NETWORK_FORWARD_BRIDGE:
+ case VIR_NETWORK_FORWARD_PRIVATE:
+ case VIR_NETWORK_FORWARD_VEPA:
+ case VIR_NETWORK_FORWARD_PASSTHROUGH:
+ if (thisName) {
+ if (VIR_STRDUP(thisIf->device.dev, thisName) < 0)
+ goto cleanup;
+ thisIf->type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV;
+ netdef->forward.nifs++;
} else {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Direct mode types require interface
names"));
- goto finish;
+ VIR_WARN("VF %zu of SRIOV PF %s couldn't be added to the "
+ "interface pool because it isn't bound "
+ "to a network driver - possibly in use elsewhere",
+ i, netdef->forward.pfs->dev);
}
- }
- else if (netdef->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) {
+ break;
+
+ case VIR_NETWORK_FORWARD_HOSTDEV:
/* VF's are always PCI devices */
- netdef->forward.ifs[i].type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI;
- netdef->forward.ifs[i].device.pci.domain = virt_fns[i]->domain;
- netdef->forward.ifs[i].device.pci.bus = virt_fns[i]->bus;
- netdef->forward.ifs[i].device.pci.slot = virt_fns[i]->slot;
- netdef->forward.ifs[i].device.pci.function = virt_fns[i]->function;
+ thisIf->type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI;
+ thisIf->device.pci.domain = thisVirtFn->domain;
+ thisIf->device.pci.bus = thisVirtFn->bus;
+ thisIf->device.pci.slot = thisVirtFn->slot;
+ thisIf->device.pci.function = thisVirtFn->function;
+ netdef->forward.nifs++;
+ break;
+
+ case VIR_NETWORK_FORWARD_NONE:
+ case VIR_NETWORK_FORWARD_NAT:
+ case VIR_NETWORK_FORWARD_ROUTE:
+ case VIR_NETWORK_FORWARD_LAST:
+ /* by definition these will never be encountered here */
+ break;
}
}
+ if (netdef->forward.nifs == 0) {
+ /* If we don't get at least one interface in the pool, declare
+ * failure
+ */
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("No usable Vf's present on SRIOV PF %s"),
+ netdef->forward.pfs->dev);
+ goto cleanup;
+ }
+
ret = 0;
- finish:
- for (i = 0; i < num_virt_fns; i++) {
- VIR_FREE(vfname[i]);
- VIR_FREE(virt_fns[i]);
+ cleanup:
+ if (ret < 0) {
+ /* free all the entries made before error */
+ for (i= 0; i < netdef->forward.nifs; i++) {
+ if (netdef->forward.ifs[i].type
+ == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV)
+ VIR_FREE(netdef->forward.ifs[i].device.dev);
+ }
+ netdef->forward.nifs = 0;
+ }
+ if (netdef->forward.nifs == 0)
+ VIR_FREE(netdef->forward.ifs);
+
+ for (i = 0; i < numVirtFns; i++) {
+ VIR_FREE(vfNames[i]);
+ VIR_FREE(virtFns[i]);
}
- VIR_FREE(vfname);
- VIR_FREE(virt_fns);
+ VIR_FREE(vfNames);
+ VIR_FREE(virtFns);
return ret;
}
--
1.9.3
3:16 p.m.
New subject: [libvirt] [PATCH 1/2] network: make networkCreateInterfacePool more robust
On 08/11/2014 12:59 PM, Laine Stump wrote:
networkCreateInterfacePool was a bit loose in its error cleanup,
which
could result in a network definition with interfaces in the pool that
were NULL. This would in turn lead to a libvirtd crash when a guest
tried to attach an interface using the network with that pool.
In particular this would happen when creating a pool to be used for
macvtap connections. macvtap needs the netdev name of the virtual
function in order to use it, and each VF only has a netdev name if it
is currently bound to a network driver. If one of the VFs of a PF
happened to be bound to the pci-stub or vfio-pci driver (indicating
it's already in use for PCI passthrough), or no driver at all, it
would have no name. In this case networkCreateInterfacePool would
return an error, but would leave the netdef->forward.nifs set to the
total number of VFs in the PF. The interface attach that triggered
calling of networkCreateInterfacePool (it uses a "lazy fill" strategy)
would simply fail, but the very next attempt to attach an interface
using the same network pool would result in a crash.
This patch refactors networkCreateInterfacePool to bring it more in
line with current coding practices (label name, use of a switch with
no default case) as well as providing the following two changes to
behavior:
1) If a VF with no netdev name is encountered, just log a warning and
continue; only fail if exactly 0 devices are found to put in the pool.
2) If the function fails, clean up any partial interface pool and set
netdef->forward.nifs to 0.
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1111455
---
src/network/bridge_driver.c | 113 ++++++++++++++++++++++++++++----------------
1 file changed, 73 insertions(+), 40 deletions(-)
ACK with nit fixed:
+ cleanup:
+ if (ret < 0) {
+ /* free all the entries made before error */
+ for (i= 0; i < netdef->forward.nifs; i++) {
Space before =
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:59 p.m.
New subject: [libvirt] [PATCH 2/2] network: populate interface pool immediately when network is started
When a network is defined with "<pf dev='xyz'/>", libvirt will
query
sysfs to learn the list of all virtual functions (VF) associated with
that Physical Function (PF) then populate the network's interface pool
accordingly. This action was previously done only when the first guest
actually requested an interface from the network. This patch changes
it to populate the pool immediately when the network is started. This
way any problems with the PF or its VFs will become apparent sooner.
Note that we can't remove the old calls to networkCreateInterfacePool
that happen whenever a guest requests an interface - doing so would be
asking for failures on hosts that had libvirt upgraded with a network
that had been started but not yet used.
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1047818
---
src/network/bridge_driver.c | 212 ++++++++++++++++++++++----------------------
1 file changed, 107 insertions(+), 105 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 918de29..4653d53 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2180,16 +2180,121 @@ static int networkShutdownNetworkVirtual(virNetworkDriverStatePtr
driver ATTRIBU
return 0;
}
+
+/* networkCreateInterfacePool:
+ * @netdef: the original NetDef from the network
+ *
+ * Creates an implicit interface pool of VF's when a PF dev is given
+ */
+static int
+networkCreateInterfacePool(virNetworkDefPtr netdef)
+{
+ size_t numVirtFns = 0;
+ char **vfNames = NULL;
+ virPCIDeviceAddressPtr *virtFns;
+
+ int ret = -1;
+ size_t i;
+
+ if ((virNetDevGetVirtualFunctions(netdef->forward.pfs->dev,
+ &vfNames, &virtFns, &numVirtFns)) <
0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get Virtual functions on %s"),
+ netdef->forward.pfs->dev);
+ goto cleanup;
+ }
+
+ netdef->forward.nifs = 0;
+ if (VIR_ALLOC_N(netdef->forward.ifs, numVirtFns) < 0)
+ goto cleanup;
+
+ for (i = 0; i < numVirtFns; i++) {
+ virPCIDeviceAddressPtr thisVirtFn = virtFns[i];
+ const char *thisName = vfNames[i];
+ virNetworkForwardIfDefPtr thisIf
+ = &netdef->forward.ifs[netdef->forward.nifs];
+
+ switch (netdef->forward.type) {
+ case VIR_NETWORK_FORWARD_BRIDGE:
+ case VIR_NETWORK_FORWARD_PRIVATE:
+ case VIR_NETWORK_FORWARD_VEPA:
+ case VIR_NETWORK_FORWARD_PASSTHROUGH:
+ if (thisName) {
+ if (VIR_STRDUP(thisIf->device.dev, thisName) < 0)
+ goto cleanup;
+ thisIf->type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV;
+ netdef->forward.nifs++;
+ } else {
+ VIR_WARN("VF %zu of SRIOV PF %s couldn't be added to the "
+ "interface pool because it isn't bound "
+ "to a network driver - possibly in use elsewhere",
+ i, netdef->forward.pfs->dev);
+ }
+ break;
+
+ case VIR_NETWORK_FORWARD_HOSTDEV:
+ /* VF's are always PCI devices */
+ thisIf->type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI;
+ thisIf->device.pci.domain = thisVirtFn->domain;
+ thisIf->device.pci.bus = thisVirtFn->bus;
+ thisIf->device.pci.slot = thisVirtFn->slot;
+ thisIf->device.pci.function = thisVirtFn->function;
+ netdef->forward.nifs++;
+ break;
+
+ case VIR_NETWORK_FORWARD_NONE:
+ case VIR_NETWORK_FORWARD_NAT:
+ case VIR_NETWORK_FORWARD_ROUTE:
+ case VIR_NETWORK_FORWARD_LAST:
+ /* by definition these will never be encountered here */
+ break;
+ }
+ }
+
+ if (netdef->forward.nifs == 0) {
+ /* If we don't get at least one interface in the pool, declare
+ * failure
+ */
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("No usable Vf's present on SRIOV PF %s"),
+ netdef->forward.pfs->dev);
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ if (ret < 0) {
+ /* free all the entries made before error */
+ for (i= 0; i < netdef->forward.nifs; i++) {
+ if (netdef->forward.ifs[i].type
+ == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV)
+ VIR_FREE(netdef->forward.ifs[i].device.dev);
+ }
+ netdef->forward.nifs = 0;
+ }
+ if (netdef->forward.nifs == 0)
+ VIR_FREE(netdef->forward.ifs);
+
+ for (i = 0; i < numVirtFns; i++) {
+ VIR_FREE(vfNames[i]);
+ VIR_FREE(virtFns[i]);
+ }
+ VIR_FREE(vfNames);
+ VIR_FREE(virtFns);
+ return ret;
+}
+
+
static int
networkStartNetworkExternal(virNetworkDriverStatePtr driver ATTRIBUTE_UNUSED,
- virNetworkObjPtr network ATTRIBUTE_UNUSED)
+ virNetworkObjPtr network)
{
/* put anything here that needs to be done each time a network of
* type BRIDGE, PRIVATE, VEPA, HOSTDEV or PASSTHROUGH is started. On
* failure, undo anything you've done, and return -1. On success
* return 0.
*/
- return 0;
+ return networkCreateInterfacePool(network->def);
}
static int networkShutdownNetworkExternal(virNetworkDriverStatePtr driver
ATTRIBUTE_UNUSED,
@@ -3600,109 +3705,6 @@ int networkRegister(void)
* "backend" function table.
*/
-/* networkCreateInterfacePool:
- * @netdef: the original NetDef from the network
- *
- * Creates an implicit interface pool of VF's when a PF dev is given
- */
-static int
-networkCreateInterfacePool(virNetworkDefPtr netdef)
-{
- size_t numVirtFns = 0;
- char **vfNames = NULL;
- virPCIDeviceAddressPtr *virtFns;
-
- int ret = -1;
- size_t i;
-
- if ((virNetDevGetVirtualFunctions(netdef->forward.pfs->dev,
- &vfNames, &virtFns, &numVirtFns)) <
0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not get Virtual functions on %s"),
- netdef->forward.pfs->dev);
- goto cleanup;
- }
-
- netdef->forward.nifs = 0;
- if (VIR_ALLOC_N(netdef->forward.ifs, numVirtFns) < 0)
- goto cleanup;
-
- for (i = 0; i < numVirtFns; i++) {
- virPCIDeviceAddressPtr thisVirtFn = virtFns[i];
- const char *thisName = vfNames[i];
- virNetworkForwardIfDefPtr thisIf
- = &netdef->forward.ifs[netdef->forward.nifs];
-
- switch (netdef->forward.type) {
- case VIR_NETWORK_FORWARD_BRIDGE:
- case VIR_NETWORK_FORWARD_PRIVATE:
- case VIR_NETWORK_FORWARD_VEPA:
- case VIR_NETWORK_FORWARD_PASSTHROUGH:
- if (thisName) {
- if (VIR_STRDUP(thisIf->device.dev, thisName) < 0)
- goto cleanup;
- thisIf->type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV;
- netdef->forward.nifs++;
- } else {
- VIR_WARN("VF %zu of SRIOV PF %s couldn't be added to the "
- "interface pool because it isn't bound "
- "to a network driver - possibly in use elsewhere",
- i, netdef->forward.pfs->dev);
- }
- break;
-
- case VIR_NETWORK_FORWARD_HOSTDEV:
- /* VF's are always PCI devices */
- thisIf->type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI;
- thisIf->device.pci.domain = thisVirtFn->domain;
- thisIf->device.pci.bus = thisVirtFn->bus;
- thisIf->device.pci.slot = thisVirtFn->slot;
- thisIf->device.pci.function = thisVirtFn->function;
- netdef->forward.nifs++;
- break;
-
- case VIR_NETWORK_FORWARD_NONE:
- case VIR_NETWORK_FORWARD_NAT:
- case VIR_NETWORK_FORWARD_ROUTE:
- case VIR_NETWORK_FORWARD_LAST:
- /* by definition these will never be encountered here */
- break;
- }
- }
-
- if (netdef->forward.nifs == 0) {
- /* If we don't get at least one interface in the pool, declare
- * failure
- */
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("No usable Vf's present on SRIOV PF %s"),
- netdef->forward.pfs->dev);
- goto cleanup;
- }
-
- ret = 0;
- cleanup:
- if (ret < 0) {
- /* free all the entries made before error */
- for (i= 0; i < netdef->forward.nifs; i++) {
- if (netdef->forward.ifs[i].type
- == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV)
- VIR_FREE(netdef->forward.ifs[i].device.dev);
- }
- netdef->forward.nifs = 0;
- }
- if (netdef->forward.nifs == 0)
- VIR_FREE(netdef->forward.ifs);
-
- for (i = 0; i < numVirtFns; i++) {
- VIR_FREE(vfNames[i]);
- VIR_FREE(virtFns[i]);
- }
- VIR_FREE(vfNames);
- VIR_FREE(virtFns);
- return ret;
-}
-
/* networkAllocateActualDevice:
* @dom: domain definition that @iface belongs to
* @iface: the original NetDef from the domain
--
1.9.3
3:17 p.m.
New subject: [libvirt] [PATCH 2/2] network: populate interface pool immediately when network is started
On 08/11/2014 12:59 PM, Laine Stump wrote:
When a network is defined with "<pf
dev='xyz'/>", libvirt will query
sysfs to learn the list of all virtual functions (VF) associated with
that Physical Function (PF) then populate the network's interface pool
accordingly. This action was previously done only when the first guest
actually requested an interface from the network. This patch changes
it to populate the pool immediately when the network is started. This
way any problems with the PF or its VFs will become apparent sooner.
Note that we can't remove the old calls to networkCreateInterfacePool
that happen whenever a guest requests an interface - doing so would be
asking for failures on hosts that had libvirt upgraded with a network
that had been started but not yet used.
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1047818
---
src/network/bridge_driver.c | 212 ++++++++++++++++++++++----------------------
1 file changed, 107 insertions(+), 105 deletions(-)
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3756
days inactive
3756
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump