netdev - Re: [PATCH net 1/2] net: bcmasp: Add missing of_node_get() before of_find_node_by

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <22c2a6ff-531f-4044-92b7-c9616642c733@huawei.com>
Date: Fri, 1 Nov 2024 17:31:16 +0800
From: "zhangzekun (A)" <zhangzekun11@...wei.com>
To: Andrew Lunn <andrew@...n.ch>
CC: <justin.chen@...adcom.com>, <florian.fainelli@...adcom.com>,
	<andrew+netdev@...n.ch>, <davem@...emloft.net>, <o.rempel@...gutronix.de>,
	<kory.maincent@...tlin.com>, <horms@...nel.org>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <netdev@...r.kernel.org>,
	<chenjun102@...wei.com>
Subject: Re: [PATCH net 1/2] net: bcmasp: Add missing of_node_get() before
 of_find_node_by_name()



在 2024/10/25 21:14, Andrew Lunn 写道:
> On Fri, Oct 25, 2024 at 10:41:22AM +0800, zhangzekun (A) wrote:
>>
>>
>> 在 2024/10/24 19:56, Andrew Lunn 写道:
>>> On Thu, Oct 24, 2024 at 09:59:08AM +0800, Zhang Zekun wrote:
>>>> of_find_node_by_name() will decrease the refcount of the device_node.
>>>> So, get the device_node before passing to it.
>>>
>>> This seems like an odd design. Why does it decrement the reference
>>> count?
>>>
>>> Rather than adding missing of_node_get() everywhere, should we be
>>> thinking about the design and fixing it to be more logical?
>>>
>>> 	Andrew
>>
>> Hi, Andrew,
>>
>> of_find* API is used as a iterater of the for loop defined in
>> "include/linux/of.h", which is already wildly used. I think is reasonable to
>> put the previous node when implement a loop, besides, the functionality has
>> been documented before the definiton of of_find* APIs.
>> The logical change of these series of APIs would require a large number of
>> conversions in the driver subsys, and I don't think it it necessary.
> 
> Do you have a rough idea how many missing gets there are because of
> this poor design?
> 
> A quick back of the envelope analysis, which will be inaccurate...
> 
> $ grep -r of_find_node_by_name | wc
>       68     348    5154
> 
> Now, a lot of these pass NULL as the node pointer:
> 
> $ grep -r of_find_node_by_name | grep NULL | wc
>       47     232    3456
> 
> so there are only about 20 call sites which are interesting. Have you
> looked at them all?
> 
> What percentage of these are not in a loop and hence don't want the
> decrement?
> 
> What percentage are broken?
> 
> We have to assume a similar number of new instances will also be
> broken, so you have an endless job of fixing these new broken cases as
> they are added.
> 
> If you found that 15 of the 20 are broken, it makes little difference
> changing the call to one which is not broken by design. If it is only
> 5 from 20 which are broken, then yes, it might be considered pointless
> churn changing them all, and you have a little job for life...
> 
> 	Andrew

Hi, Andrew,

There are about 10/20 call sites might have this problem, spreading in 
six files. May be we can fix these problems instead of adding a new API?

Thanks,
Zekun