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

[Andrew Donnellan <ajd@linux.ibm.com>]

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?

Either way - I think there needs to be a comment above
get_dvsec_vendor0() documenting when an additional reference needs to
be released.

> ---
>  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;
>  }
>  

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited