linux-kernel - Re: [RFC PATCH 0/5] of: automate of_node_put()

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]

Date: Thu, 1 Feb 2024 13:20:31 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: linux-iio@...r.kernel.org, Rob Herring <robh@...nel.org>,
	Frank Rowand <frowand.list@...il.com>, linux-kernel@...r.kernel.org,
	Julia Lawall <Julia.Lawall@...ia.fr>,
	Nicolas Palix <nicolas.palix@...g.fr>,
	Sumera Priyadarsini <sylphrenadin@...il.com>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nuno Sá <nuno.sa@...log.com>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [RFC PATCH 0/5] of: automate of_node_put() - new approach to
 loops.

On Sun, Jan 28, 2024 at 04:05:37PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> 
> +CC includes peopleinterested in property.h equivalents to minimize
> duplication of discussion.  Outcome of this discussion will affect:
> https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
> 
> In discussion of previous approach with Rob Herring we talked about various
> ways to avoid a disconnect between the declaration of the __free(device_node)
> and the first non NULL assignment. Making this connection clear is useful for 2
> reasons:
> 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> 2) Avoids disconnect between how cleanup is to be done and how the reference
>    was acquired in the first place.
> 
> https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
> 
> The options we discussed are:
> 
> 1) Ignore this issue and merge original set.
> 
> 2) Always put the declaration just before the for loop and don't set it NULL.
> 
> {
> 	int ret;
> 
> 	ret = ... and other fun code.
> 
> 	struct device_node *child __free(device_node);
> 	for_each_child_of_node(np, child) {
> 	}
> }
> 
> This works but careful review is needed to ensure that this unusual pattern is
> followed.  We don't set it to NULL as the loop will do that anyway if there are
> no child nodes, or the loop finishes without an early break or return.
> 
> 3) Introduced the pointer to auto put device_node only within the
>    for loop scope.
> 
> +#define for_each_child_of_node_scoped(parent, child) \
> +	for (struct device_node *child __free(device_node) =		\
> +	     of_get_next_child(parent, NULL);				\
> +	     child != NULL;						\

Just

	     child;							\

> +	     child = of_get_next_available_child(parent, child))
> +
> 
> This series is presenting option 3.  I only implemented this loop out of
> all the similar ones and it is only compile tested.
> 
> Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> a struct device_node *child.  I can't see a way around that other than option 2
> above, but all suggestions welcome.  Note that if a conversion leaves an
> 'external' struct device_node *child variable, in many cases the compiler
> will catch that as an unused variable. We don't currently run shaddow
> variable detection in normal kernel builds, but that could also be used
> to catch such bugs.

-- 
With Best Regards,
Andy Shevchenko