Re: [RFC PATCH 1/4] of: Add cleanup.h based autorelease via __free(device_node) markings.

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 21 Dec 2023 10:54:34 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 20 Dec 2023 16:11:44 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Sun, Dec 17, 2023 at 06:46:45PM +0000, Jonathan Cameron wrote:  
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > The recent addition of scope based cleanup support to the kernel
> > > provides a convenient tool to reduce the chances of leaking reference
> > > counts where of_node_put() should have been called in an error path.
> > > 
> > > This enables
> > > 	struct device_node *child __free(device_node) = NULL;
> > > 
> > > 	for_each_child_of_node(np, child) {
> > > 		if (test)
> > > 			return test;
> > > 	}
> > > 
> > > with no need for a manual call of of_node_put()
> > > 
> > > In this simile example the gains are small but there are some very    
> > 
> > typo
> >   
> > > complex error handling cases burried in these loops that wil be
> > > greatly simplified by enabling early returns with out the need
> > > for this manual of_node_put() call.    
> > 
> > Neat!
> > 
> > I guess that now that the coccinelle check has fixed many, we can update 
> > it to the new way and start fixing them all again. We should update the 
> > coccinelle script with the new way. See 
> > scripts/coccinelle/iterators/for_each_child.cocci.  
> 
> If the holiday season gets dull enough I'll take a look at updating that
> as well. Been a long time since I last messed with coccinelle.
> 
> Given this is just a simplification rather than a fix, there would be no rush
> to convert things over but we definitely don't want the coccinelle script
> to generate lots of false positives.  + we should perhaps add a
> script to try and catch the opposite (double free) as a result of
> using this automated cleanup.
Hi Rob,

As things currently stand the script doesn't trigger on a
struct device_node __free(device_node); (which is wrong anyway)
or
struct device_node __free(device_node) = NULL;

So we at least don't cause a flurry of false positives via these
changes.

I'm not keen to add an upstream check to encourage conversion over
to this new approach simply because there is no great rush to do it
and it's easy enough to use grep to find potential targets today.

Also strongly motivated by the fact I don't really have time to
learn coccinelle (however useful that would be in the long run!)

As such I'll tidy these up a bit and send out a non RFC version with
cover letter additions to mention we don't cause false positives and
that a coccinelle script to find candidates might make sense in the
longer term.  It may also make sense to add checks that we don't manually
release the node on error paths without making sure to steal the pointer
(which sets it to NULL to avoid problems).

+CC various Coccinelle folk even though I'm proposing to not do any
coccinelle scripting for now.

Jonathan