linux-kernel - Re: [PATCH] libfdt: place new nodes & properties after the parent's ones

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: <20200205054508.GG60221@umbus.fritz.box>
Date:   Wed, 5 Feb 2020 16:45:08 +1100
From:   David Gibson <david@...son.dropbear.id.au>
To:     Marek Szyprowski <m.szyprowski@...sung.com>
Cc:     devicetree-compiler@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [PATCH] libfdt: place new nodes & properties after the parent's
 ones

On Tue, Feb 04, 2020 at 01:58:44PM +0100, Marek Szyprowski wrote:
> While applying dt-overlays using libfdt code, the order of the applied
> properties and sub-nodes is reversed. This should not be a problem in
> ideal world (mainline), but this matters for some vendor specific/custom
> dtb files. This can be easily fixed by the little change to libfdt code:
> any new properties and sub-nodes should be added after the parent's node
> properties and subnodes.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@...sung.com>

I'm not convinced this is a good idea.

First, anything that relies on the order of properties or subnodes in
a dtb is deeply, fundamentally broken.  That can't even really be a
problem with a dtb file itself, only with the code processing it.

I'm also concerned this could have a negative performance impact,
since it has to skip over a bunch of existing things before adding the
new one.  On the other hand, that may be offset by the fact that it
will reduce the amount of stuff that needs to be memmove()ed later on.

> ---
>  libfdt/fdt_rw.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 8795947..88c5930 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -189,19 +189,27 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
>  			     int len, struct fdt_property **prop)
>  {
>  	int proplen;
> -	int nextoffset;
> +	int offset, nextoffset;
>  	int namestroff;
>  	int err;
>  	int allocated;
> +	uint32_t tag;
>  
>  	if ((nextoffset = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
>  		return nextoffset;
>  
> +	/* Try to place the new property after the parent's properties */
> +	fdt_next_tag(fdt, nodeoffset, &nextoffset); /* skip the BEGIN_NODE */
> +	do {
> +		offset = nextoffset;
> +		tag = fdt_next_tag(fdt, offset, &nextoffset);
> +	} while ((tag == FDT_PROP) || (tag == FDT_NOP));
> +
>  	namestroff = fdt_find_add_string_(fdt, name, &allocated);
>  	if (namestroff < 0)
>  		return namestroff;
>  
> -	*prop = fdt_offset_ptr_w_(fdt, nextoffset);
> +	*prop = fdt_offset_ptr_w_(fdt, offset);
>  	proplen = sizeof(**prop) + FDT_TAGALIGN(len);
>  
>  	err = fdt_splice_struct_(fdt, *prop, 0, proplen);
> @@ -321,6 +329,7 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset,
>  	struct fdt_node_header *nh;
>  	int offset, nextoffset;
>  	int nodelen;
> +	int depth = 0;
>  	int err;
>  	uint32_t tag;
>  	fdt32_t *endtag;
> @@ -333,12 +342,21 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset,
>  	else if (offset != -FDT_ERR_NOTFOUND)
>  		return offset;
>  
> -	/* Try to place the new node after the parent's properties */
> +	/* Try to place the new node after the parent's subnodes */
>  	fdt_next_tag(fdt, parentoffset, &nextoffset); /* skip the BEGIN_NODE */
>  	do {
> +again:
>  		offset = nextoffset;
>  		tag = fdt_next_tag(fdt, offset, &nextoffset);
> -	} while ((tag == FDT_PROP) || (tag == FDT_NOP));
> +		if (depth && tag == FDT_END_NODE) {
> +			depth--;
> +			goto again;
> +		}
> +		if (tag == FDT_BEGIN_NODE) {
> +			depth++;
> +			goto again;
> +		}
> +	} while (depth || (tag == FDT_PROP) || (tag == FDT_NOP));
>  
>  	nh = fdt_offset_ptr_w_(fdt, offset);
>  	nodelen = sizeof(*nh) + FDT_TAGALIGN(namelen+1) + FDT_TAGSIZE;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)