[libvirt] [PATCH v2 0/2] Don't crash in virNodeDeviceDefFormat()
Changes from [v1]: * fix all callers of virPCIGetPhysicalFunction() * document virPCIGetPhysicalFunction() [v1] https://www.redhat.com/archives/libvir-list/2016-May/msg01681.html Andrea Bolognani (2): Document virPCIGetPhysicalFunction() and fix its callers conf: nodedev: Set PCI_PHYSICAL_FUNCTION flag more carefully src/conf/node_device_conf.c | 4 ++-- src/node_device/node_device_linux_sysfs.c | 10 ++++++++-- src/util/virpci.c | 18 ++++++++++++++++-- 3 files changed, 26 insertions(+), 6 deletions(-) -- 2.5.5
Commit c8b1a83605e4 changed the function, making it impossible for callers to be able to tell whether a non-negative return value means "physical function address found and parsed correctly" or "couldn't find corresponding physical function". The important difference between the two being that, in the latter case, the returned pointer is NULL and should never, ever be dereferenced. In order to cope with these changes, the callers have to be updated. Some documentation has also been added to the function, so that callers know how to handle the return value and returned data properly. --- src/node_device/node_device_linux_sysfs.c | 10 ++++++++-- src/util/virpci.c | 18 ++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 24a6a2e..b337d2d 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -154,19 +154,25 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; - if (!virPCIGetPhysicalFunction(sysfsPath, &data->pci_dev.physical_function)) + ret = virPCIGetPhysicalFunction(sysfsPath, + &data->pci_dev.physical_function); + if (ret < 0) + goto out; + + if (data->pci_dev.physical_function) data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; ret = virPCIGetVirtualFunctions(sysfsPath, &data->pci_dev.virtual_functions, &data->pci_dev.num_virtual_functions, &data->pci_dev.max_virtual_functions); if (ret < 0) - return ret; + goto out; if (data->pci_dev.num_virtual_functions > 0 || data->pci_dev.max_virtual_functions > 0) data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; + out: return ret; } diff --git a/src/util/virpci.c b/src/util/virpci.c index 3d18bb3..e8dc987 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2478,8 +2478,19 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) return bdf; } -/* - * Returns Physical function given a virtual function +/** + * virPCIGetPhysicalFunction: + * @vf_sysfs_path: sysfs path for the virtual function + * @pf: where to store the physical function's address + * + * Given @vf_sysfs_path, this function will store the pointer + * to a newly-allocated virPCIDeviceAddress in @pf. + * + * @pf might be NULL if @vf_sysfs_path does not point to a + * virtual function. If it's not NULL, then it should be + * freed by the caller when no longer needed. + * + * Returns: >=0 on success, <0 on failure */ int virPCIGetPhysicalFunction(const char *vf_sysfs_path, @@ -2719,6 +2730,9 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, if (virPCIGetPhysicalFunction(vf_sysfs_device_path, &pf_config_address) < 0) return ret; + if (!pf_config_address) + return ret; + if (virPCIDeviceAddressGetSysfsFile(pf_config_address, &pf_sysfs_device_path) < 0) { -- 2.5.5
On 05/24/2016 07:11 AM, Andrea Bolognani wrote:
Commit c8b1a83605e4 changed the function, making it impossible for callers to be able to tell whether a non-negative return value means "physical function address found and parsed correctly" or "couldn't find corresponding physical function".
The important difference between the two being that, in the latter case, the returned pointer is NULL and should never, ever be dereferenced.
In order to cope with these changes, the callers have to be updated. Some documentation has also been added to the function, so that callers know how to handle the return value and returned data properly. --- src/node_device/node_device_linux_sysfs.c | 10 ++++++++-- src/util/virpci.c | 18 ++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 24a6a2e..b337d2d 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -154,19 +154,25 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
- if (!virPCIGetPhysicalFunction(sysfsPath, &data->pci_dev.physical_function)) + ret = virPCIGetPhysicalFunction(sysfsPath, + &data->pci_dev.physical_function); + if (ret < 0) + goto out;
I avoid adding "out:" labels, and use "cleanup:" instead, even if there currently isn't any cleanup done there (and if there isn't any cleanup to be done, why goto anyway?)
+ + if (data->pci_dev.physical_function) data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
ret = virPCIGetVirtualFunctions(sysfsPath, &data->pci_dev.virtual_functions, &data->pci_dev.num_virtual_functions, &data->pci_dev.max_virtual_functions); if (ret < 0) - return ret; + goto out;
if (data->pci_dev.num_virtual_functions > 0 || data->pci_dev.max_virtual_functions > 0) data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
+ out: return ret; }
diff --git a/src/util/virpci.c b/src/util/virpci.c index 3d18bb3..e8dc987 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2478,8 +2478,19 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) return bdf; }
-/* - * Returns Physical function given a virtual function +/** + * virPCIGetPhysicalFunction: + * @vf_sysfs_path: sysfs path for the virtual function + * @pf: where to store the physical function's address + * + * Given @vf_sysfs_path, this function will store the pointer + * to a newly-allocated virPCIDeviceAddress in @pf. + * + * @pf might be NULL if @vf_sysfs_path does not point to a + * virtual function. If it's not NULL, then it should be + * freed by the caller when no longer needed. + * + * Returns: >=0 on success, <0 on failure
As long as you're here, just to be safe, how about having virPCIGetPhysicalFunction set "*pf = NULL" at the beginning so that it's NULL even if there is an OOM error and the caller hasn't initialized it? (Yeah, I know that will "never ever" happen. But as long as we're being boy scouts, we might as well do it up right :-))
*/ int virPCIGetPhysicalFunction(const char *vf_sysfs_path, @@ -2719,6 +2730,9 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, if (virPCIGetPhysicalFunction(vf_sysfs_device_path, &pf_config_address) < 0) return ret;
+ if (!pf_config_address) + return ret; + if (virPCIDeviceAddressGetSysfsFile(pf_config_address, &pf_sysfs_device_path) < 0) {
ACK.
On Tue, 2016-05-24 at 11:22 -0400, Laine Stump wrote:
On 05/24/2016 07:11 AM, Andrea Bolognani wrote:
Commit c8b1a83605e4 changed the function, making it impossible for callers to be able to tell whether a non-negative return value means "physical function address found and parsed correctly" or "couldn't find corresponding physical function". The important difference between the two being that, in the latter case, the returned pointer is NULL and should never, ever be dereferenced. In order to cope with these changes, the callers have to be updated. Some documentation has also been added to the function, so that callers know how to handle the return value and returned data properly. --- src/node_device/node_device_linux_sysfs.c | 10 ++++++++-- src/util/virpci.c | 18 ++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 24a6a2e..b337d2d 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -154,19 +154,25 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; - if (!virPCIGetPhysicalFunction(sysfsPath, &data->pci_dev.physical_function)) + ret = virPCIGetPhysicalFunction(sysfsPath, + &data->pci_dev.physical_function); + if (ret < 0) + goto out; I avoid adding "out:" labels, and use "cleanup:" instead, even if there currently isn't any cleanup done there (and if there isn't any cleanup to be done, why goto anyway?)
I like having a single exit point for the function, plus if the function ends up having to allocate (and consequently release) memory in the future it doesn't have to be changed. ... Which is a good argument for using "cleanup" from the very beginning, I guess. Changed :)
+ + if (data->pci_dev.physical_function) data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; ret = virPCIGetVirtualFunctions(sysfsPath, &data->pci_dev.virtual_functions, &data->pci_dev.num_virtual_functions, &data->pci_dev.max_virtual_functions); if (ret < 0) - return ret; + goto out; if (data->pci_dev.num_virtual_functions > 0 || data->pci_dev.max_virtual_functions > 0) data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; + out: return ret; } diff --git a/src/util/virpci.c b/src/util/virpci.c index 3d18bb3..e8dc987 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2478,8 +2478,19 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) return bdf; } -/* - * Returns Physical function given a virtual function +/** + * virPCIGetPhysicalFunction: + * @vf_sysfs_path: sysfs path for the virtual function + * @pf: where to store the physical function's address + * + * Given @vf_sysfs_path, this function will store the pointer + * to a newly-allocated virPCIDeviceAddress in @pf. + * + * @pf might be NULL if @vf_sysfs_path does not point to a + * virtual function. If it's not NULL, then it should be + * freed by the caller when no longer needed. + * + * Returns: >=0 on success, <0 on failure As long as you're here, just to be safe, how about having virPCIGetPhysicalFunction set "*pf = NULL" at the beginning so that it's NULL even if there is an OOM error and the caller hasn't initialized it? (Yeah, I know that will "never ever" happen. But as long as we're being boy scouts, we might as well do it up right :-))
Probably not that important, since you're supposed to check the return value first - if the function returns <0 and you go ahead and poke at the data anyway, then you're just asking for trouble. I've added it anyway because it can't possibly hurt :)
*/ int virPCIGetPhysicalFunction(const char *vf_sysfs_path, @@ -2719,6 +2730,9 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, if (virPCIGetPhysicalFunction(vf_sysfs_device_path, &pf_config_address) < 0) return ret; + if (!pf_config_address) + return ret; + if (virPCIDeviceAddressGetSysfsFile(pf_config_address, &pf_sysfs_device_path) < 0) { ACK.
I've addressed your comments and pushed the changes after chopping up into smaller and more focused commits. Thanks for reviewing! -- Andrea Bolognani Software Engineer - Virtualization Team
Instead of setting the flag before parsing the PCI address, set it afterwards. This ensure we can never end up in a situation where the flag has been set but pci_dev.physical_function has not been filled in. --- src/conf/node_device_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index aed95d5..a23d8ef 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1293,8 +1293,6 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, if (VIR_ALLOC(data->pci_dev.physical_function) < 0) goto out; - data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; - if (!address) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing address in 'phys_function' capability")); @@ -1304,6 +1302,8 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, if (virPCIDeviceAddressParseXML(address, data->pci_dev.physical_function) < 0) goto out; + + data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; } else if (STREQ(type, "virt_functions")) { int naddresses; -- 2.5.5
On 05/24/2016 07:11 AM, Andrea Bolognani wrote:
Instead of setting the flag before parsing the PCI address, set it afterwards. This ensure we can never end up in a situation where the flag has been set but pci_dev.physical_function has not been filled in. --- src/conf/node_device_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index aed95d5..a23d8ef 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1293,8 +1293,6 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, if (VIR_ALLOC(data->pci_dev.physical_function) < 0) goto out;
- data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; - if (!address) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing address in 'phys_function' capability")); @@ -1304,6 +1302,8 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, if (virPCIDeviceAddressParseXML(address, data->pci_dev.physical_function) < 0) goto out; + + data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; } else if (STREQ(type, "virt_functions")) { int naddresses;
ACK, although it currently doesn't matter (I checked and made sure that the memory at pci_dev.physical_function is free'd even if the PHYSICAL_FUNCTION flag isn't set)
participants (2)
-
Andrea Bolognani -
Laine Stump