Re: [libvirt] [PATCHv2] pci: properly handle out-of-order SRIOV virtual functions

This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1025397 When libvirt creates the list of an SRIOV Physical Function's (PF) Virtual Functions (VF), it assumes that the order of "virtfn*" links returned by readdir() from the PF's sysfs directory is already in the correct order. Experience has shown that this is not always the case. This results in 1) incorrect assumptions made by consumers of the output of the virt_functions list of virsh nodedev-dumpxml, and 2) setting MAC address and vlan tag on the wrong VF (since libvirt uses netlink to set mac address and vlan tag, netlink requires the VF#, and the function virPCIGetVirtualFunctionIndex() returns the wrong index due to the improperly ordered VF list). See the bugzilla report for an example of this improper behavior. The solution provided by this patch is for virPCIGetVirtualFunctions to first gather all the "virtfn*" names from the PFs sysfs directory, then allocate a virtual_functions array large enough to hold all entries, and finally to create a device entry for each virtfn* name and place it into the virtual_functions array at the proper index (based on interpreting the value following "virtfn" in the name). Checks are introduced to ensure that 1) the virtfn list given in sysfs is not sparse (ie, there are no indexes larger than the length of the list), and that no two virtfn* entries decode to the same index. --- Difference from V1: Based on jtomko's review, truncate trailing NULLs in virtual_functions array, and actually check for NULLs in the middle (a sparse array) and generate an error in that case, as we already claimed to do in the commit message. (Sorry for the lack of inter-diff, but the only changes from V1 are the addition of the lines starting with "truncate any trailing nulls" and continuing until the next VIR_DEBUG.) src/util/virpci.c | 115 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 25 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 148631f..f744c8b 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2379,6 +2379,8 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, return ret; } +static const char *virtfnPrefix = "virtfn"; + /* * Returns virtual functions of a physical function */ @@ -2393,6 +2395,8 @@ virPCIGetVirtualFunctions(const char *sysfs_path, struct dirent *entry = NULL; char *device_link = NULL; char errbuf[64]; + char **virtfnNames = NULL; + size_t nVirtfnNames = 0; VIR_DEBUG("Attempting to get SR IOV virtual functions for device" "with sysfs path '%s'", sysfs_path); @@ -2411,51 +2415,112 @@ virPCIGetVirtualFunctions(const char *sysfs_path, while ((entry = readdir(dir))) { if (STRPREFIX(entry->d_name, "virtfn")) { - virPCIDeviceAddress *config_addr = NULL; + char *tempName; - if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { - virReportOOMError(); + /* save these to sort into virtual_functions array */ + if (VIR_STRDUP(tempName, entry->d_name) < 0) + goto error; + if (VIR_APPEND_ELEMENT(virtfnNames, nVirtfnNames, tempName) < 0) { + VIR_FREE(tempName); goto error; } + } + }

- VIR_DEBUG("Number of virtual functions: %d", - *num_virtual_functions); + /* pre-allocate because we will be filling in out of order */ + if (nVirtfnNames && VIR_ALLOC_N(*virtual_functions, nVirtfnNames) < 0) + goto error; + *num_virtual_functions = nVirtfnNames; - if (virPCIGetDeviceAddressFromSysfsLink(device_link, - &config_addr) != - SRIOV_FOUND) { - VIR_WARN("Failed to get SRIOV function from device " - "link '%s'", device_link); - VIR_FREE(device_link); - continue; - } + for (i = 0; i < nVirtfnNames; i++) { + virPCIDeviceAddress *config_addr = NULL; + unsigned int virtfnIndex; - if (VIR_REALLOC_N(*virtual_functions, - *num_virtual_functions + 1) < 0) { - VIR_FREE(config_addr); - goto error; - } + if (virBuildPath(&device_link, sysfs_path, virtfnNames[i]) < 0) { + virReportOOMError(); + goto error; + } + + if (virStrToLong_ui(virtfnNames[i] + strlen(virtfnPrefix), + 0, 10, &virtfnIndex) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid virtual function link name '%s' " + "in physical function directory '%s'"), + virtfnNames[i], sysfs_path); + goto error; + }

+ + if (virtfnIndex >= nVirtfnNames) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' larger than " + "total count of virtual functions %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + } +

+ if ((*virtual_functions)[virtfnIndex]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' " + "generates duplicate index %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + }

- (*virtual_functions)[*num_virtual_functions] = config_addr; - (*num_virtual_functions)++; + if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) != + SRIOV_FOUND) {

+ VIR_WARN("Failed to get SRIOV function from device link '%s'", + device_link); VIR_FREE(device_link); + continue; }

+ + VIR_DEBUG("Found virtual function %d", virtfnIndex); + (*virtual_functions)[virtfnIndex] = config_addr; + VIR_FREE(device_link); }

+ /* truncate any trailing nulls due to files having names starting + * with "virtfn" which weren't actually a link to a virtual + * function. + */ + while (*num_virtual_functions && + !(*virtual_functions)[(*num_virtual_functions) - 1]) + (*num_virtual_functions)--; + + /* failure of realloc when shrinking is safe */ + ignore_value(VIR_REALLOC_N(*virtual_functions, *num_virtual_functions)); +

+ /* if there are any NULLs left in the array, fail and log an error + * - the virtual function array cannot be sparse. + */ + for (i = 0; i < *num_virtual_functions; i++) { + if (!(*virtual_functions)[*num_virtual_functions]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing virtual function link for VF %zu " + "in physical function directory '%s'"), + i, sysfs_path);

+ goto error; + } + } + + VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions);