[PATCH] of: refcount leak when phandle_cache entry replaced

[PATCH] of: refcount leak when phandle_cache entry replaced

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

of_find_node_by_phandle() does not do an of_node_put() of the existing
node in a phandle cache entry when that node is replaced by a new node.

Reported-by: Rob Herring <robh+dt@kernel.org>
Fixes: b8a9ac1a5b99 ("of: of_node_get()/of_node_put() nodes held in phandle cache")
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

Checkpatch will warn about a line over 80 characters.  Let me know
if that bothers you.

 drivers/of/base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index db7fbc0c0893..b57a57752294 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1261,6 +1261,8 @@ struct device_node *of_find_node_by_phandle(phandle handle)
 			if (np->phandle == handle &&
 			    !of_node_check_flag(np, OF_DETACHED)) {
 				if (phandle_cache) {
+					if (phandle_cache[masked_handle])
+						of_node_put(phandle_cache[masked_handle]);
 					/* will put when removed from cache */
 					of_node_get(np);
 					phandle_cache[masked_handle] = np;
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH] of: refcount leak when phandle_cache entry replaced

[Rob Herring]

On Tue, 10 Dec 2019 02:14:53 -0600, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> of_find_node_by_phandle() does not do an of_node_put() of the existing
> node in a phandle cache entry when that node is replaced by a new node.
> 
> Reported-by: Rob Herring <robh+dt@kernel.org>
> Fixes: b8a9ac1a5b99 ("of: of_node_get()/of_node_put() nodes held in phandle cache")
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> 
> Checkpatch will warn about a line over 80 characters.  Let me know
> if that bothers you.
> 
>  drivers/of/base.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Applied, thanks.

Rob

Re: [PATCH] of: refcount leak when phandle_cache entry replaced

[Frank Rowand]

On 12/11/19 2:18 PM, Rob Herring wrote:
> On Tue, 10 Dec 2019 02:14:53 -0600, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> of_find_node_by_phandle() does not do an of_node_put() of the existing
>> node in a phandle cache entry when that node is replaced by a new node.
>>
>> Reported-by: Rob Herring <robh+dt@kernel.org>
>> Fixes: b8a9ac1a5b99 ("of: of_node_get()/of_node_put() nodes held in phandle cache")
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> Checkpatch will warn about a line over 80 characters.  Let me know
>> if that bothers you.
>>
>>  drivers/of/base.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
> 
> Applied, thanks.
> 
> Rob
> 

If the rework patch of the cache that you posted shortly after accepting
my patch, then my patch becomes not needed and is just extra noise in the
history.  Once your patch finishes review (I am assuming it probably
will), then my patch should be reverted.

-Frank

Re: [PATCH] of: refcount leak when phandle_cache entry replaced

[Rob Herring]

On Thu, Dec 12, 2019 at 5:17 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 12/11/19 2:18 PM, Rob Herring wrote:
> > On Tue, 10 Dec 2019 02:14:53 -0600, frowand.list@gmail.com wrote:
> >> From: Frank Rowand <frank.rowand@sony.com>
> >>
> >> of_find_node_by_phandle() does not do an of_node_put() of the existing
> >> node in a phandle cache entry when that node is replaced by a new node.
> >>
> >> Reported-by: Rob Herring <robh+dt@kernel.org>
> >> Fixes: b8a9ac1a5b99 ("of: of_node_get()/of_node_put() nodes held in phandle cache")
> >> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> >> ---
> >>
> >> Checkpatch will warn about a line over 80 characters.  Let me know
> >> if that bothers you.
> >>
> >>  drivers/of/base.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >
> > Applied, thanks.
> >
> > Rob
> >
>
> If the rework patch of the cache that you posted shortly after accepting
> my patch, then my patch becomes not needed and is just extra noise in the
> history.  Once your patch finishes review (I am assuming it probably
> will), then my patch should be reverted.

The question is what to backport: nothing, this patch or mine? My
thought was to apply this mainly to backport. If you're fine with
nothing or mine, then we can drop it. I'm a bit nervous marking mine
for stable.

Rob

Re: [PATCH] of: refcount leak when phandle_cache entry replaced

[Frank Rowand]

On 12/12/19 8:00 AM, Rob Herring wrote:
> On Thu, Dec 12, 2019 at 5:17 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 12/11/19 2:18 PM, Rob Herring wrote:
>>> On Tue, 10 Dec 2019 02:14:53 -0600, frowand.list@gmail.com wrote:
>>>> From: Frank Rowand <frank.rowand@sony.com>
>>>>
>>>> of_find_node_by_phandle() does not do an of_node_put() of the existing
>>>> node in a phandle cache entry when that node is replaced by a new node.
>>>>
>>>> Reported-by: Rob Herring <robh+dt@kernel.org>
>>>> Fixes: b8a9ac1a5b99 ("of: of_node_get()/of_node_put() nodes held in phandle cache")
>>>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>>>> ---
>>>>
>>>> Checkpatch will warn about a line over 80 characters.  Let me know
>>>> if that bothers you.
>>>>
>>>>  drivers/of/base.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>
>>> Applied, thanks.
>>>
>>> Rob
>>>
>>
>> If the rework patch of the cache that you posted shortly after accepting
>> my patch, then my patch becomes not needed and is just extra noise in the
>> history.  Once your patch finishes review (I am assuming it probably
>> will), then my patch should be reverted.
> 
> The question is what to backport: nothing, this patch or mine? My
> thought was to apply this mainly to backport. If you're fine with
> nothing or mine, then we can drop it. I'm a bit nervous marking mine
> for stable.
> 
> Rob
> 

Your rework patch is slightly larger than what is preferred for stable,
but it is more likely that future patches to the files in the rework
patch will be able to be applied to stable.  So I am happy with
either nothing or your patch.

-Frank