Re: [PATCH] ocxl: fix pci device refcount leak when calling get_function_0()

[Yang Yingliang <yangyingliang@huawei.com>]

On 2022/11/21 14:31, Andrew Donnellan wrote:
> On Sat, 2022-11-19 at 22:44 +0800, Yang Yingliang wrote:
>> As comment of pci_get_domain_bus_and_slot() says, it returns
>> a pci device with refcount increment, so when finish using it,
>> pci_dev_put() needs be called.
>>
>> In get_dvsec_vendor0(), in normal path, the returned pci device
>> is passed to dev0, so after using dev0 in the callers, it need
>> be put, in error path, pci_dev_put() also needs be called.
>>
>> pci_get_domain_bus_and_slot() is called when PCI_FUNC() returns
>> non-zero, check this before put.
>>
>> Fixes: 87db7579ebd5 ("ocxl: control via sysfs whether the FPGA is
>> reloaded on a link reset")
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> It might be neater to take an additional reference on dev in
> get_dvsec_vendor0() in the case where dev is function 0, which would
> mean you could call pci_dev_put() unconditionally in the callers?
Yes, I think it's a better way.
>
> Either way - I think there needs to be a comment above
> get_dvsec_vendor0() documenting when an additional reference needs to
> be released.
I will send a v2 with the above changing.

Thanks,
Yang
>
>> ---
>>   drivers/misc/ocxl/config.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
>> index e401a51596b9..4da5a2b8514c 100644
>> --- a/drivers/misc/ocxl/config.c
>> +++ b/drivers/misc/ocxl/config.c
>> @@ -196,16 +196,21 @@ static int read_dvsec_vendor(struct pci_dev
>> *dev)
>>   static int get_dvsec_vendor0(struct pci_dev *dev, struct pci_dev
>> **dev0,
>>                               int *out_pos)
>>   {
>> +       bool need_put;
>>          int pos;
>>   
>>          if (PCI_FUNC(dev->devfn) != 0) {
>>                  dev = get_function_0(dev);
>>                  if (!dev)
>>                          return -1;
>> +               need_put = true;
>>          }
>>          pos = find_dvsec(dev, OCXL_DVSEC_VENDOR_ID);
>> -       if (!pos)
>> +       if (!pos) {
>> +               if (need_put)
>> +                       pci_dev_put(dev);
>>                  return -1;
>> +       }
>>          *dev0 = dev;
>>          *out_pos = pos;
>>          return 0;
>> @@ -222,6 +227,8 @@ int ocxl_config_get_reset_reload(struct pci_dev
>> *dev, int *val)
>>   
>>          pci_read_config_dword(dev0, pos +
>> OCXL_DVSEC_VENDOR_RESET_RELOAD,
>>                                &reset_reload);
>> +       if (PCI_FUNC(dev->devfn) != 0)
>> +               pci_dev_put(dev0);
>>          *val = !!(reset_reload & BIT(0));
>>          return 0;
>>   }
>> @@ -243,6 +250,8 @@ int ocxl_config_set_reset_reload(struct pci_dev
>> *dev, int val)
>>                  reset_reload &= ~BIT(0);
>>          pci_write_config_dword(dev0, pos +
>> OCXL_DVSEC_VENDOR_RESET_RELOAD,
>>                                 reset_reload);
>> +       if (PCI_FUNC(dev->devfn) != 0)
>> +               pci_dev_put(dev0);
>>          return 0;
>>   }
>>