* [PATCH] ocxl: fix pci device refcount leak when calling get_function_0()
@ 2022-11-19 14:44 Yang Yingliang
2022-11-21 6:28 ` Christophe Leroy
2022-11-21 6:31 ` Andrew Donnellan
0 siblings, 2 replies; 5+ messages in thread
From: Yang Yingliang @ 2022-11-19 14:44 UTC (permalink / raw)
To: fbarrat, ajd, arnd, gregkh; +Cc: linuxppc-dev, yangyingliang
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;
}
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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] ocxl: fix pci device refcount leak when calling get_function_0()
2022-11-19 14:44 [PATCH] ocxl: fix pci device refcount leak when calling get_function_0() Yang Yingliang
@ 2022-11-21 6:28 ` Christophe Leroy
2022-11-21 6:50 ` Yang Yingliang
2022-11-21 6:31 ` Andrew Donnellan
1 sibling, 1 reply; 5+ messages in thread
From: Christophe Leroy @ 2022-11-21 6:28 UTC (permalink / raw)
To: Yang Yingliang, fbarrat@linux.ibm.com, ajd@linux.ibm.com,
arnd@arndb.de, gregkh@linuxfoundation.org
Cc: linuxppc-dev@lists.ozlabs.org
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;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ocxl: fix pci device refcount leak when calling get_function_0()
2022-11-21 6:28 ` Christophe Leroy
@ 2022-11-21 6:50 ` Yang Yingliang
0 siblings, 0 replies; 5+ messages in thread
From: Yang Yingliang @ 2022-11-21 6:50 UTC (permalink / raw)
To: Christophe Leroy, fbarrat@linux.ibm.com, ajd@linux.ibm.com,
arnd@arndb.de, gregkh@linuxfoundation.org
Cc: linuxppc-dev@lists.ozlabs.org, yangyingliang
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;
>> }
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ocxl: fix pci device refcount leak when calling get_function_0()
2022-11-19 14:44 [PATCH] ocxl: fix pci device refcount leak when calling get_function_0() Yang Yingliang
2022-11-21 6:28 ` Christophe Leroy
@ 2022-11-21 6:31 ` Andrew Donnellan
2022-11-21 6:56 ` Yang Yingliang
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Donnellan @ 2022-11-21 6:31 UTC (permalink / raw)
To: Yang Yingliang, fbarrat, arnd, gregkh; +Cc: linuxppc-dev
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ocxl: fix pci device refcount leak when calling get_function_0()
2022-11-21 6:31 ` Andrew Donnellan
@ 2022-11-21 6:56 ` Yang Yingliang
0 siblings, 0 replies; 5+ messages in thread
From: Yang Yingliang @ 2022-11-21 6:56 UTC (permalink / raw)
To: Andrew Donnellan, fbarrat, arnd, gregkh; +Cc: linuxppc-dev, yangyingliang
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;
>> }
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-21 6:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-19 14:44 [PATCH] ocxl: fix pci device refcount leak when calling get_function_0() Yang Yingliang
2022-11-21 6:28 ` Christophe Leroy
2022-11-21 6:50 ` Yang Yingliang
2022-11-21 6:31 ` Andrew Donnellan
2022-11-21 6:56 ` Yang Yingliang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox