linux-kernel - Re: Usage of for_each_child_of

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: <20131023071006.GA7708@ulmo.nvidia.com>
Date:	Wed, 23 Oct 2013 09:10:07 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Rob Herring <robherring2@...il.com>
Cc:	Guenter Roeck <linux@...ck-us.net>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Usage of for_each_child_of_node()

On Sat, Oct 12, 2013 at 10:15:03PM -0500, Rob Herring wrote:
> On Sat, Oct 12, 2013 at 3:54 PM, Guenter Roeck <linux@...ck-us.net> wrote:
> > Hi all,
> >
> > for_each_child_of_node() and similar functions increase the refcount
> > on each returned node and expect the caller to release the node by
> > calling of_node_put() when done.
> >
> > Looking through the kernel code, it appears this is hardly ever done,
> > if at all. Some code even calls of_node_get() on returned nodes again.
> >
> > I guess this doesn't matter in cases where devicetree is a static entity.
> > However, this is not (or no longer) the case with devicetree overlays,
> > or more generically in cases where devicetree nodes are added and
> > removed dynamically.
> >
> > Fundamental question: Would patches to fix this problem be accepted upstream
> > ?
> 
> Certainly.
> 
> > Or, of course, stepping a bit back: Am I missing something essential ?
> 
> No. I think this is frequently wrong since it typically doesn't matter
> for static entries as you mention.

Actually, I think it actually happens to be correct most of the time.
The reason is that for_each_child_of_node() internally calls the
of_get_next_child() to iterate over all children. And that function
already calls of_node_put() on the "previous" node. So if all the code
does is to iterate over all nodes to query them, then all should be
fine.

The only case where you actually need to drop the reference on a node is
if you break out of the loop (so that of_get_next_child() will not be
called). But that's usually the case when you need to perform some
operation on the node, in which case it is the right thing to hold on to
a reference until you're done with the node.

Thierry

Content of type "application/pgp-signature" skipped