linux-kernel - Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9beaf6ea-14bb-1944-2372-eb615f04f9e3@nvidia.com>
Date:   Fri, 30 Nov 2018 11:10:08 +0800
From:   Wei Ni <wni@...dia.com>
To:     Thierry Reding <thierry.reding@...il.com>
CC:     <daniel.lezcano@...aro.org>, <linux-tegra@...r.kernel.org>,
        <rui.zhang@...el.com>, <edubezval@...il.com>, <srikars@...dia.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor
 register



On 30/11/2018 1:13 AM, Thierry Reding wrote:
> On Thu, Nov 29, 2018 at 01:55:02PM +0800, Wei Ni wrote:
>> On 28/11/2018 6:25 PM, Thierry Reding wrote:
>>> On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
> [...]
>>>> +	bool ret = false;
>>>> +	struct of_phandle_args sensor_specs;
>>>> +	struct device_node *np, *sensor_np;
>>>> +
>>>> +	np = of_find_node_by_name(NULL, "thermal-zones");
>>>> +	if (!np)
>>>> +		return ret;
>>>> +
>>>> +	sensor_np = of_get_next_child(np, NULL);
>>>> +	for_each_available_child_of_node(np, sensor_np) {
>>>
>>> Aren't we leaking np here? I think we need of_node_put() after
>>> of_get_next_child() to make sure the reference to the "thermal-zones"
>>> node is properly released.
>>
>> No, we will not leak np here. Because the
>> for_each_available_child_of_node will call
>> of_get_next_available_child(), which will call of_node_put(prev) to
>> decrease refcount of the prev node. So we just need to of_node_put the
>> last node after break from this for block.
> 
> Okay, looks like I misinterpreted what you were doing there. I thought
> that for_each_available_child_of_node() took the child as first argument
> and the parent as second and therefore np would be overwritten by the
> first assignment in the macro.
> 
> But looking at this more closely I think there's something else wrong
> here. for_each_available_child_of_node() is defined as:
> 
> 	for_each_available_child_of_node(parent, child)
> 
> so in the above, np will be the parent and sensor_np the child. Why do
> you have to do
> 
> 	sensor_np = of_get_next_child(np, NULL);
> 
> ? That's already done as part of the loop in the macro, right? So does
> that not mean we get two references and we leak the first one? Can the
> above not simply been dropped?

It's so sorry, it's my mistake, we should remove this line, it was my
develop code, forgot to remove it.
Will fix it in next version.

Thanks.

> 
> Thierry
>