* [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset()
@ 2019-02-14 6:41 Huang Zijiang
2019-02-14 17:29 ` David Miller
2019-02-15 10:52 ` John Garry
0 siblings, 2 replies; 6+ messages in thread
From: Huang Zijiang @ 2019-02-14 6:41 UTC (permalink / raw)
To: yisen.zhuang
Cc: salil.mehta, davem, lipeng321, liuyonglong, yuehaibing, keescook,
wangxi11, netdev, linux-kernel, wang.yi59, Huang Zijiang
The of_find_device_by_node() takes a reference to the underlying device
structure, we should release that reference.
Signed-off-by: Huang Zijiang <huang.zijiang@zte.com.cn>
---
drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
index 14d7ec7..697d929 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
@@ -3081,6 +3081,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset)
dsaf_dev = dev_get_drvdata(&pdev->dev);
if (!dsaf_dev) {
dev_err(&pdev->dev, "dsaf_dev is NULL\n");
+ put_device(&pdev->dev);
return -ENODEV;
}
@@ -3088,6 +3089,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset)
if (AE_IS_VER1(dsaf_dev->dsaf_ver)) {
dev_err(dsaf_dev->dev, "%s v1 chip doesn't support RoCE!\n",
dsaf_dev->ae_dev.name);
+ put_device(&pdev->dev);
return -ENODEV;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset()
2019-02-14 6:41 [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset() Huang Zijiang
@ 2019-02-14 17:29 ` David Miller
2019-02-15 10:52 ` John Garry
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2019-02-14 17:29 UTC (permalink / raw)
To: huang.zijiang
Cc: yisen.zhuang, salil.mehta, lipeng321, liuyonglong, yuehaibing,
keescook, wangxi11, netdev, linux-kernel, wang.yi59
From: Huang Zijiang <huang.zijiang@zte.com.cn>
Date: Thu, 14 Feb 2019 14:41:45 +0800
> The of_find_device_by_node() takes a reference to the underlying device
> structure, we should release that reference.
>
> Signed-off-by: Huang Zijiang <huang.zijiang@zte.com.cn>
Applied, thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset()
2019-02-14 6:41 [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset() Huang Zijiang
2019-02-14 17:29 ` David Miller
@ 2019-02-15 10:52 ` John Garry
2019-02-15 11:25 ` Salil Mehta
1 sibling, 1 reply; 6+ messages in thread
From: John Garry @ 2019-02-15 10:52 UTC (permalink / raw)
To: Huang Zijiang, yisen.zhuang
Cc: salil.mehta, davem, lipeng321, liuyonglong, yuehaibing, keescook,
wangxi11, netdev, linux-kernel, wang.yi59, Linuxarm
On 14/02/2019 06:41, Huang Zijiang wrote:
> The of_find_device_by_node() takes a reference to the underlying device
> structure, we should release that reference.
of_find_device_by_node() is not called for every path, so is this change
proper:
/* find the platform device corresponding to fwnode */
if (is_of_node(dsaf_fwnode)) {
pdev = of_find_device_by_node(to_of_node(dsaf_fwnode));
} else if (is_acpi_device_node(dsaf_fwnode)) {
pdev = hns_dsaf_find_platform_device(dsaf_fwnode);
} else {
pr_err("fwnode is neither OF or ACPI type\n");
return -EINVAL;
}
/* check if we were a success in fetching pdev */
if (!pdev) {
pr_err("couldn't find platform device for node\n");
return -ENODEV;
}
/* retrieve the dsaf_device from the driver data */
dsaf_dev = dev_get_drvdata(&pdev->dev);
if (!dsaf_dev) {
dev_err(&pdev->dev, "dsaf_dev is NULL\n");
return -ENODEV;
}
John
>
> Signed-off-by: Huang Zijiang <huang.zijiang@zte.com.cn>
> ---
> drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> index 14d7ec7..697d929 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> @@ -3081,6 +3081,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset)
> dsaf_dev = dev_get_drvdata(&pdev->dev);
> if (!dsaf_dev) {
> dev_err(&pdev->dev, "dsaf_dev is NULL\n");
> + put_device(&pdev->dev);
> return -ENODEV;
> }
>
> @@ -3088,6 +3089,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset)
> if (AE_IS_VER1(dsaf_dev->dsaf_ver)) {
> dev_err(dsaf_dev->dev, "%s v1 chip doesn't support RoCE!\n",
> dsaf_dev->ae_dev.name);
> + put_device(&pdev->dev);
> return -ENODEV;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset()
2019-02-15 10:52 ` John Garry
@ 2019-02-15 11:25 ` Salil Mehta
2019-02-15 12:00 ` John Garry
0 siblings, 1 reply; 6+ messages in thread
From: Salil Mehta @ 2019-02-15 11:25 UTC (permalink / raw)
To: John Garry, Huang Zijiang, Zhuangyuzeng (Yisen)
Cc: davem@davemloft.net, lipeng (Y), liuyonglong, yuehaibing,
keescook@chromium.org, wangxi (M), netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, wang.yi59@zte.com.cn, Linuxarm
> From: John Garry
> Sent: Friday, February 15, 2019 10:52 AM
>
> On 14/02/2019 06:41, Huang Zijiang wrote:
> > The of_find_device_by_node() takes a reference to the underlying device
> > structure, we should release that reference.
>
> of_find_device_by_node() is not called for every path, so is this change proper:
It looks okay to me and with the suggested device reference is being
released from every possible error leg in the function. Could you be
more specific which error path is not being addressed?
> /* find the platform device corresponding to fwnode */
> if (is_of_node(dsaf_fwnode)) {
> pdev = of_find_device_by_node(to_of_node(dsaf_fwnode));
This will get the reference to the device, which needs to be
released later.
> } else if (is_acpi_device_node(dsaf_fwnode)) {
> pdev = hns_dsaf_find_platform_device(dsaf_fwnode);
This will also get the reference to the device when bus_find_device()
gets called and returns the 'device'. Therefore, this needs to be
released as well using put_device()
> } else {
> pr_err("fwnode is neither OF or ACPI type\n");
> return -EINVAL;
> }
>
> /* check if we were a success in fetching pdev */
> if (!pdev) {
> pr_err("couldn't find platform device for node\n");
> return -ENODEV;
> }
>
> /* retrieve the dsaf_device from the driver data */
> dsaf_dev = dev_get_drvdata(&pdev->dev);
> if (!dsaf_dev) {
> dev_err(&pdev->dev, "dsaf_dev is NULL\n");
> return -ENODEV;
> }
>
> John
[...]
> > --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> > +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> > @@ -3081,6 +3081,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset)
> > dsaf_dev = dev_get_drvdata(&pdev->dev);
> > if (!dsaf_dev) {
> > dev_err(&pdev->dev, "dsaf_dev is NULL\n");
> > + put_device(&pdev->dev);
This looks okay.
> > return -ENODEV;
> > }
> >
> > @@ -3088,6 +3089,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset)
> > if (AE_IS_VER1(dsaf_dev->dsaf_ver)) {
> > dev_err(dsaf_dev->dev, "%s v1 chip doesn't support RoCE!\n",
> > dsaf_dev->ae_dev.name);
> > + put_device(&pdev->dev);
This looks okay.
Salil.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset()
2019-02-15 11:25 ` Salil Mehta
@ 2019-02-15 12:00 ` John Garry
2019-02-18 17:54 ` Salil Mehta
0 siblings, 1 reply; 6+ messages in thread
From: John Garry @ 2019-02-15 12:00 UTC (permalink / raw)
To: Salil Mehta, Huang Zijiang, Zhuangyuzeng (Yisen)
Cc: davem@davemloft.net, lipeng (Y), liuyonglong, yuehaibing,
keescook@chromium.org, wangxi (M), netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, wang.yi59@zte.com.cn, Linuxarm
On 15/02/2019 11:25, Salil Mehta wrote:
>> From: John Garry
>> Sent: Friday, February 15, 2019 10:52 AM
>>
>> On 14/02/2019 06:41, Huang Zijiang wrote:
>>> The of_find_device_by_node() takes a reference to the underlying device
>>> structure, we should release that reference.
>>
>> of_find_device_by_node() is not called for every path, so is this change proper:
>
>
> It looks okay to me and with the suggested device reference is being
> released from every possible error leg in the function. Could you be
> more specific which error path is not being addressed?
>
>
>
>> /* find the platform device corresponding to fwnode */
>> if (is_of_node(dsaf_fwnode)) {
>> pdev = of_find_device_by_node(to_of_node(dsaf_fwnode));
>
>
> This will get the reference to the device, which needs to be
> released later.
>
>
>> } else if (is_acpi_device_node(dsaf_fwnode)) {
>> pdev = hns_dsaf_find_platform_device(dsaf_fwnode);
>
>
> This will also get the reference to the device when bus_find_device()
> gets called and returns the 'device'. Therefore, this needs to be
> released as well using put_device()
OK, I see. That's non-obvious.
And so the commit message was simply incomplete.
>
So who finally drops this reference? This patch only seems to release on
error path. I checked the callers and they don't seem to.
John
>
>
>> } else {
>> pr_err("fwnode is neither OF or ACPI type\n");
>> return -EINVAL;
>> }
>>
>> /* check if we were a success in fetching pdev */
>> if (!pdev) {
>> pr_err("couldn't find platform device for node\n");
>> return -ENODEV;
>> }
>>
>> /* retrieve the dsaf_device from the driver data */
>> dsaf_dev = dev_get_drvdata(&pdev->dev);
>> if (!dsaf_dev) {
>> dev_err(&pdev->dev, "dsaf_dev is NULL\n");
>> return -ENODEV;
>> }
>>
>> John
>
>
> [...]
>
>>> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
>>> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
>>> @@ -3081,6 +3081,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset)
>>> dsaf_dev = dev_get_drvdata(&pdev->dev);
>>> if (!dsaf_dev) {
>>> dev_err(&pdev->dev, "dsaf_dev is NULL\n");
>>> + put_device(&pdev->dev);
>
>
> This looks okay.
>
>
>>> return -ENODEV;
>>> }
>>>
>>> @@ -3088,6 +3089,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset)
>>> if (AE_IS_VER1(dsaf_dev->dsaf_ver)) {
>>> dev_err(dsaf_dev->dev, "%s v1 chip doesn't support RoCE!\n",
>>> dsaf_dev->ae_dev.name);
>>> + put_device(&pdev->dev);
>
>
> This looks okay.
>
>
> Salil.
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset()
2019-02-15 12:00 ` John Garry
@ 2019-02-18 17:54 ` Salil Mehta
0 siblings, 0 replies; 6+ messages in thread
From: Salil Mehta @ 2019-02-18 17:54 UTC (permalink / raw)
To: John Garry, Huang Zijiang, Zhuangyuzeng (Yisen)
Cc: davem@davemloft.net, lipeng (Y), liuyonglong, yuehaibing,
keescook@chromium.org, wangxi (M), netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, wang.yi59@zte.com.cn, Linuxarm
> From: John Garry
> Sent: Friday, February 15, 2019 12:00 PM
>
> On 15/02/2019 11:25, Salil Mehta wrote:
> >> From: John Garry
> >> Sent: Friday, February 15, 2019 10:52 AM
> >>
> >> On 14/02/2019 06:41, Huang Zijiang wrote:
> >>> The of_find_device_by_node() takes a reference to the underlying device
> >>> structure, we should release that reference.
> >>
> >> of_find_device_by_node() is not called for every path, so is this change proper:
> >
> >
> > It looks okay to me and with the suggested device reference is being
> > released from every possible error leg in the function. Could you be
> > more specific which error path is not being addressed?
> >
> >> /* find the platform device corresponding to fwnode */
> >> if (is_of_node(dsaf_fwnode)) {
> >> pdev = of_find_device_by_node(to_of_node(dsaf_fwnode));
> >
> >
> > This will get the reference to the device, which needs to be
> > released later.
> >
> >
> >> } else if (is_acpi_device_node(dsaf_fwnode)) {
> >> pdev = hns_dsaf_find_platform_device(dsaf_fwnode);
> >
> >
> > This will also get the reference to the device when bus_find_device()
> > gets called and returns the 'device'. Therefore, this needs to be
> > released as well using put_device()
>
> OK, I see. That's non-obvious.
>
> And so the commit message was simply incomplete.
>
> So who finally drops this reference? This patch only seems to release on
> error path. I checked the callers and they don't seem to.
Yes, in the positive leg the put_device() was missing. Thanks for identifying!
I have floated the patch to fix it https://www.lkml.org/lkml/2019/2/18/1161
Salil.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-18 17:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-14 6:41 [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset() Huang Zijiang
2019-02-14 17:29 ` David Miller
2019-02-15 10:52 ` John Garry
2019-02-15 11:25 ` Salil Mehta
2019-02-15 12:00 ` John Garry
2019-02-18 17:54 ` Salil Mehta
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).