[libvirt] [PATCH 1/2] nodedev: sysfs: Set PCI_PHYSICAL_FUNCTION flag more carefully

[Laine Stump]

On 05/23/2016 07:53 AM, Andrea Bolognani wrote:
> The flag was set if virPCIGetPhysicalFunction() did not return
> an error; unfortunately, the 'physfn' sysfs file not being
> present is not considered an error, which means that the flag
> was set pretty much unconditionally.

This behavior is the result of commit c8b1a83605:

   https://www.redhat.com/archives/libvir-list/2016-May/msg01348.html

Prior to this, virPCIGetPhysicalFunction had returned the return value 
from virPCIGetDeviceAddressFromSysfsLink(), which would be -1 if the 
physfn file didn't exist. That patch discards the return value and 
hardcodes a "return 0".

I think that virPCIGetPhysicalFunction should be restored to its 
original glory (No, I didn't write it). Either that, or the other caller 
to virPCIGetPhysicalFunction (virPCIGetVirtualFunctionInfo) should also 
be fixed so that it doesn't crash due to the changed semantics of 
virPCIGetPhysicalFunction.

(A more correct fix may be to change the semantics of 
virPCIGetPhysicalFunction() to return -1 only in the case of a bonafide 
error (OOM, failure to parse the contents of an existing physfn file), 
but that would also require modifying 
virPCIGetDeviceAddressFromSysfsLink() to return 0/-1 rather than a 
virPCIDeviceAddresPtr so that we can tell the difference between "no 
physfn" and "there was a physfn but I encountered some other error". In 
other words - 1) revert commit c1faf309 (part of the same "coverity 
fixes" series as x8b1a83605), then 2) adjust the function to set *bdf = 
0 and return 0 when the physfn file doesn't exist.)


>
> This, in turn, caused libvirtd to crash in
> virNodeDeviceDefFormat(), where the presence of the flag
> is considered a reliable indicator of the fact that
> pci_dev.physical_function has been filled in.
>
> Change the code to check pci_dev.physical_function before
> setting the flag.
> ---
>   src/node_device/node_device_linux_sysfs.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 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;
>   }
>