linux-kernel - Re: [PATCH 5/6] OF: Utility helper functions for dynamic nodes

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

Message-Id: <20140625090940.783DAC40D39@trevor.secretlab.ca>
Date:	Wed, 25 Jun 2014 10:09:40 +0100
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Alexander Sverdlin <alexander.sverdlin@....com>,
	Ioan Nicu <ioan.nicu.ext@....com>,
	ext Pantelis Antoniou <pantelis.antoniou@...sulko.com>
Cc:	Rob Herring <robherring2@...il.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Matt Porter <matt.porter@...aro.org>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alison Chaiken <Alison_Chaiken@...tor.com>,
	Dinh Nguyen <dinh.linux@...il.com>,
	Jan Lubbe <jluebbe@...net.de>, Michael Stickel <ms@...able.de>,
	Guenter Roeck <linux@...ck-us.net>,
	Dirk Behme <dirk.behme@...il.com>,
	Alan Tull <delicious.quinoa@...il.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Michael Bohan <mbohan@...eaurora.org>,
	Michal Simek <monstr@...str.eu>,
	Matt Ranostay <mranostay@...il.com>,
	Joel Becker <jlbec@...lplan.org>, devicetree@...r.kernel.org,
	Wolfram Sang <wsa@...-dreams.de>, linux-i2c@...r.kernel.org,
	Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
	linux-kernel@...r.kernel.org, Pete Popov <pete.popov@...sulko.com>,
	Dan Malek <dan.malek@...sulko.com>,
	Georgi Vlaev <georgi.vlaev@...sulko.com>
Subject: Re: [PATCH 5/6] OF: Utility helper functions for dynamic nodes

On Tue, 24 Jun 2014 10:10:01 +0200, Alexander Sverdlin <alexander.sverdlin@....com> wrote:
> Hi Pantelis, Grant,
> 
> On 23/06/14 20:33, Ioan Nicu wrote:
> >>> On 22/06/14 11:40, ext Pantelis Antoniou wrote:
> >>>> Introduce helper functions for working with the live DT tree,
> >>>> all of them related to dynamically adding/removing nodes and
> >>>> properties.
> >>>>
> >>>> __of_copy_property() copies a property dynamically
> >>>> __of_create_empty_node() creates an empty node
> >>>>
> >>>> Bug fix about prop->len == 0 by Ionut Nicu <ioan.nicu.ext@....com>
> >>>
> >>> Are you sure about this? (see below...)
> >>>
> > 
> > Alexander is right, my fix was lost even though it's mentioned in this patch.
> > 
> >>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
> >>>> ---
> >>
> >> [snip]
> >>>> +
> >>>> +	if (prop->length > 0) {
> >>>        ^^^^^^^^^^^^^^^^^^^^^
> >>> Seems, that length==0 case will still produce value==NULL results,
> >>> which will brake some checks in the kernel... Or am I missing something in
> >>> the new version?
> >>>
> >>
> >> prop->value will be set to NULL, and length will be set to zero (kzalloc).
> >> This is a normal zero length property.
> >>
> >> I don't know of any place in the kernel accessing the value if prop->length==0
> >>
> > 
> > We have a simple use case. We have an overlay which adds an interrupt controller.
> > If you look in drivers/of/irq.c, in of_irq_parse_raw():
> > 
> > [...]
> > 	/* Now start the actual "proper" walk of the interrupt tree */
> > 	while (ipar != NULL) {
> > 		/* Now check if cursor is an interrupt-controller and if it is
> > 		 * then we are done
> > 		 */
> > 		if (of_get_property(ipar, "interrupt-controller", NULL) !=
> 
> We have to define, if it's allowed for an empty property to have NULL value.
> Several places in the kernel use of_get_property() to check for property existence.
> We either have to make a tree-wide patch and replace of_get_property() with of_find_property() in those cases,
> or ensure value != NULL in this copy function...
> 
> Grant, what do you think?

I think it's a good cleanup, but until it happens, any new code needs to
match the behaviour of fdt.c. In that case, making the value point to an
empty string is a sane choice.  If you do the legwork of finding callers
and fixing them up, then I'll apply it. Once done we can make the
following change to fdt.c:

- 			pp->value = (__be32 *)p;
+ 			pp->value = sz ? (__be32 *)p : 0;

pdt.c similarly needs to be updated.

I'm not overly worried though. That code has been in place for a very
long time, so there is no rush. It can exist a while longer.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/