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

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3e4868af91c2141ccdfaba39445b3d83d2a35961..d95d0c43ba02f1e664c6794c9846ffda0c01a2e7 100644 (file)
--- 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;
 }