[PATCH 5/7] util: change call sequence for virPCIDeviceFindCapabilityOffset()

[Laine Stump]

On 12/9/20 4:18 AM, Daniel P. Berrangé wrote:
> On Tue, Dec 08, 2020 at 08:37:43PM -0500, Laine Stump wrote:
>> Previously there was no way to differentiate between this function 1)
>> encountering an error while reading the pci config, and 2) determining
>> that the device in question is a conventional PCI device, and so has
>> no Express Capabilities.
>>
>> The difference between these two conditions is important, because an
>> unprivileged libvirtd will be unable to read all of the pci config (it
>> can only read the first 64 bytes, and will get ENOENT when it tries to
>> seek past that limit) even though the device is in fact a PCIe device.
>>
>> This patch just changes virPCIDeviceFindCapabilityOffset() to put the
>> determined offset into an argument of the function (rather than
>> sending it back as the return value), and to return the standard "0 on
>> success, -1 on failure". Failure is determined by checking the value
>> of errno after each attemptd read of the config file (which can only
>> work reliably if errno is reset to 0 before each read, and after
>> virPCIDeviceFindCapabilityOffset() has finished examining it).
>>
>> (NB: if the config file is read successfully, but no Express
>> Capabilities are found, then the function returns success, but the
>> returned offset will be 0 (which is an impossible offset for Express
>> Capabilities, and so easily recognizeable).
>>
>> An upcoming patch will take advantage of the change made here.
>>
>> Signed-off-by: Laine Stump <laine at redhat.com>
>> ---
>>   src/util/virpci.c | 42 +++++++++++++++++++++++++++++++-----------
>>   1 file changed, 31 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index 9109fb4f3a..2f99bb93bd 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -329,6 +329,7 @@ virPCIDeviceRead(virPCIDevicePtr dev,
>>                    unsigned int buflen)
>>   {
>>       memset(buf, 0, buflen);
>> +    errno = 0;
>>   
>>       if (lseek(cfgfd, pos, SEEK_SET) != pos ||
>>           saferead(cfgfd, buf, buflen) != buflen) {
>> @@ -483,19 +484,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
>>       return ret;
>>   }
>>
>
> I think there should be some API docs added here to describe the
> semantics this method is trying to achieve wrt unprivileged
> usage and pci vs pcie.


There is something about that in patch 6/7, but I think all of these 
functions could benefit from documenting their API (e.g. the fact that 
errno may be set on return from virPCIDeviceReadNN(), since the 
functions themselves directly return the value that is read, and so are 
unable indicate an error in the return value (I considered doing that, 
but wanted a less invasive change, and there are 20-30 callers of those 
functions, most of which wouldn't otherwise need to be changed).


I'll add in some API docublurbs and resend the entire series just for 
simplicity...


>
>> -static uint8_t
>> +static int
>>   virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
>>                                    int cfgfd,
>> -                                 unsigned int capability)
>> +                                 unsigned int capability,
>> +                                 unsigned int *offset)
>>   {
>>       uint16_t status;
>>       uint8_t pos;
>>   
>> +    *offset = 0; /* assume failure (*nothing* can be at offset 0) */
>> +
>>       status = virPCIDeviceRead16(dev, cfgfd, PCI_STATUS);
>> -    if (!(status & PCI_STATUS_CAP_LIST))
>> -        return 0;
>> +    if (errno != 0 || !(status & PCI_STATUS_CAP_LIST))
>> +        goto error;
>>   
>>       pos = virPCIDeviceRead8(dev, cfgfd, PCI_CAPABILITY_LIST);
>> +    if (errno != 0)
>> +        goto error;
>>   
>>       /* Zero indicates last capability, capabilities can't
>>        * be in the config space header and 0xff is returned
>> @@ -506,18 +512,31 @@ virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
>>        */
>>       while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) {
>>           uint8_t capid = virPCIDeviceRead8(dev, cfgfd, pos);
>> +        if (errno != 0)
>> +            goto error;
>> +
>>           if (capid == capability) {
>>               VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x",
>>                         dev->id, dev->name, capability, pos);
>> -            return pos;
>> +            *offset = pos;
>> +            return 0;
>>           }
>>   
>>           pos = virPCIDeviceRead8(dev, cfgfd, pos + 1);
>> +        if (errno != 0)
>> +            goto error;
>>       }
>>   
>> -    VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id, dev->name, capability);
>> + error:
>> +    VIR_DEBUG("%s %s: failed to find cap 0x%.2x (%s)",
>> +              dev->id, dev->name, capability, g_strerror(errno));
>>   
>> -    return 0;
>> +    /* reset errno in case the failure was due to insufficient
>> +     * privileges to read the entire PCI config file
>> +     */
>> +    errno = 0;
>> +
>> +    return -1;
>>   }
>>   
>>   static unsigned int
>> @@ -553,7 +572,7 @@ static bool
>>   virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
>>   {
>>       uint32_t caps;
>> -    uint8_t pos;
>> +    unsigned int pos;
>>       g_autofree char *path = NULL;
>>       int found;
>>   
>> @@ -575,7 +594,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
>>        * the same thing, except for conventional PCI
>>        * devices. This is not common yet.
>>        */
>> -    pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF);
>> +    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
>> +
>>       if (pos) {
>>           caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
>>           if (caps & PCI_AF_CAP_FLR) {
>> @@ -885,8 +905,8 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
>>   static int
>>   virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
>>   {
>> -    dev->pcie_cap_pos   = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP);
>> -    dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM);
>> +    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP, &dev->pcie_cap_pos);
>> +    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM, &dev->pci_pm_cap_pos);
>>       dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
>>       dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
>>   
>> -- 
>> 2.28.0
>>
> Regards,
> Daniel