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: <0c9ea6c2-535d-4ce8-aea1-7523b5304635@lunn.ch>
Date: Fri, 25 Oct 2024 15:14:26 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "zhangzekun (A)" <zhangzekun11@...wei.com>
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()

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