| Message ID | 20221119144433.2454759-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ocxl: fix pci device refcount leak when calling get_function_0() | expand |
Le 19/11/2022 à 15:44, Yang Yingliang a écrit : > 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> > --- > 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; Why introduce that 'need_put' boolean ? Why not just use PCI_FUNC(dev->devfn) != 0 as you do in other places ? > } > 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) The != 0 is useless, just do: if (PCI_FUNC(dev->devfn)) > + 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) Same > + pci_dev_put(dev0); > return 0; > } >
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; > } >
Hi, On 2022/11/21 14:28, Christophe Leroy wrote: > > Le 19/11/2022 à 15:44, Yang Yingliang a écrit : >> 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> >> --- >> 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; > Why introduce that 'need_put' boolean ? Why not just use > PCI_FUNC(dev->devfn) != 0 as you do in other places ? The 'dev' is reassigned by get_function_0(), it is not the origin one. Thanks, Yang > >> } >> 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) > The != 0 is useless, just do: > > if (PCI_FUNC(dev->devfn)) > >> + 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) > Same > >> + pci_dev_put(dev0); >> return 0; >> } >>
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; >> } >>
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; }
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> --- drivers/misc/ocxl/config.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)