Re: [PATCH v3 02/15] libfdt: Don't assume the root node is available at offset 0

[David Gibson]

On Mon, Sep 07, 2026 at 06:46:41PM +0200, Herve Codina wrote:
> Hi David,
> 
> On Wed, 2 Sep 2026 17:06:03 +1000
> David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> ...
> > > 
> > > Ok, I will update fdt_check_node_offset_() to have it updating its offset
> > > parameter to the real offset of the root node when its value is 0.
> > > 
> > > Based on this update, will see where it goes. I mean, impacts on callers, if
> > > it simplifies things or not, if the offset update needs also to be propagate
> > > to caller's parameter or any other similar point that we can see during the
> > > implementation.
> > > 
> > > Having something implemented and available in a patch will be the best to
> > > compare changes and impacts related to fdt_check_node_offset_() update.
> > > 
> > > Here we have a version of handling offset 0 vs real root node without any
> > > offset update done in fdt_check_node_offset_(). In the next iteration we will
> > > have the version with update done in fdt_check_node_offset_().
> > > 
> > > I think the golden rules to follow on this point is "keep it as simple as
> > > possible".  
> > 
> > Agreed.  Feel free to repost just the NOP before root patches on their
> > own.  At this time, frequent small series is easier for me to tackle
> > than occasional large series.
> > 
> 
> I've moved forward on the fdt_check_node_offset_() update.
> 
> The new fdt_check_node_offset_() looks like this:
> --- 8< ---
> int fdt_check_node_offset_(const void *fdt, int *offset)
> {
> 	int nextoffset;
> 
> 	if (!can_assume(VALID_INPUT)
> 	    && ((*offset < 0) || (*offset % FDT_TAGSIZE)))
> 		return -FDT_ERR_BADOFFSET;
> 
> 	if (*offset == 0) {
> 		*offset = fdt_root_offset(fdt);
> 		if (*offset < 0)
> 			return *offset;
> 	}
> 
> 	if (fdt_next_tag(fdt, *offset, &nextoffset) != FDT_BEGIN_NODE)
> 		return -FDT_ERR_BADOFFSET;
> 
> 	return nextoffset;
> }
> --- 8< ---

That looks fine.  If it was an exposed function I'd want to look for a
better interface, but as an internal function it's fine.

> If the given offset is 0, fdt_check_node_offset_() considers we want to check
> the root node and so update offset to the real root node offset.
> 
> Ok, I still need some fdt_root_offset() calls from some other parts but that's
> not my main issue.
> 
> My main issue comes with orphan nodes in addons. fdt_check_node_offset_() is
> called with offset pointing to an orphan node and this offset can be 0.
> 
> The offset 0 seen by fdt_check_node_offset_() can be the "fake" offset of a root
> node and in that case fdt_check_node_offset_() should update offset to the real
> root node offset but it can also be the offset of the orphan node we want to check
> and in that case the offset should not be updated.
> 
> fdt_check_node_offset_() cannot determine whether or not the offset should be
> updated.

Hrm.  But if offset 0 has an orphan node, we can see that with the
FDT_BEGIN_NODE or FDT_BEGIN_NODE_REF there, right?

> With orphan nodes in the loop, fdt_check_node_offset_() becomes:
> --- 8< ---
> int fdt_check_node_offset_(const void *fdt, int *offset)
> {
>         int nextoffset;
>         uint32_t tag;
> 
>         if (!can_assume(VALID_INPUT)
>             && ((*offset < 0) || (*offset % FDT_TAGSIZE)))
>                 return -FDT_ERR_BADOFFSET;
> 
>         if (*offset == 0) {
> #pragma message "We have a problem!"
>                 /*
>                  * An orphan node can be present at offset 0.
>                  * In that case, looking for the root node may or may not be
>                  * correct.
>                  * Indeed is offset = 0 requested because we want the root
>                  * node and sadly an orphan node is available at offset 0 or
>                  * is it requested because we want to really check the orphan
>                  * node available at offset 0. How to determine the correct
>                  * case?
>                  */
>                 tag = fdt_next_tag(fdt, *offset, &nextoffset);
>                 if (tag == FDT_BEGIN_NODE || tag == FDT_BEGIN_NODE_REF)
>                         return nextoffset;
> 
>                 *offset = fdt_root_offset(fdt);
>                 if (*offset < 0)
>                         return *offset;
>         }
> 
>         tag = fdt_next_tag(fdt, *offset, &nextoffset);
>         if (tag != FDT_BEGIN_NODE && tag != FDT_BEGIN_NODE_REF)
>                 return -FDT_ERR_BADOFFSET;
> 
>         return nextoffset;
> }
> --- 8< ---

Right.. like that.  Except simpler would be to have fdt_root_offset()
stop when it sees a FDT_BEGIN_NODE_REF as well as a FDT_BEGIN_NODE.
Or maybe that should be, say, fdt_root_offset_(), and
fdt_root_offset() will wrap it to return an error on an addon tree
with no root.

> Of course, extra complexity could be added such as an additional parameter but
> I don't think it would make sense.
> 
> The simplest way to handle the case is to not update the offset in
> fdt_check_node_offset_() and let callers to pass the offset pointing to the
> real node offset expected to be checked. This is my initial proposal.

Even with the bulky fdt_check_node_offset_() above that doesn't seem
simpler to me.  fdt_check_node_offset_() is largish, but the
complexity is all in one place rather than distributed everywhere.

> Of course, I am still open to other ideas.
> 
> For information, without addons and orphan nodes, just to compare against my
> original proposal, the full patch ("libfdt: Don't assume the root node is
> available at offset 0") with the offset update done in fdt_check_node_offset_()
> becomes:
> --- 8< ---
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 56d4dcb2..d4d4b64b 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -228,16 +228,24 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  	return tag;
>  }
>  
> -int fdt_check_node_offset_(const void *fdt, int offset)
> +int fdt_check_node_offset_(const void *fdt, int *offset)
>  {
> +	int nextoffset;
> +
>  	if (!can_assume(VALID_INPUT)
> -	    && ((offset < 0) || (offset % FDT_TAGSIZE)))
> +	    && ((*offset < 0) || (*offset % FDT_TAGSIZE)))
>  		return -FDT_ERR_BADOFFSET;
>  
> -	if (fdt_next_tag(fdt, offset, &offset) != FDT_BEGIN_NODE)
> +	if (*offset == 0) {
> +		*offset = fdt_root_offset(fdt);
> +		if (*offset < 0)
> +			return *offset;
> +	}
> +
> +	if (fdt_next_tag(fdt, *offset, &nextoffset) != FDT_BEGIN_NODE)
>  		return -FDT_ERR_BADOFFSET;
>  
> -	return offset;
> +	return nextoffset;
>  }
>  
>  int fdt_check_prop_offset_(const void *fdt, int offset)
> @@ -252,13 +260,38 @@ int fdt_check_prop_offset_(const void *fdt, int offset)
>  	return offset;
>  }
>  
> +int fdt_root_offset(const void *fdt)
> +{
> +	int nextoffset = 0;
> +	int offset;
> +	uint32_t tag;
> +
> +	do {
> +		offset = nextoffset;
> +		tag = fdt_next_tag(fdt, offset, &nextoffset);
> +		switch (tag) {
> +		case FDT_END_NODE:
> +		case FDT_PROP:
> +			return -FDT_ERR_BADSTRUCTURE;
> +
> +		case FDT_BEGIN_NODE:
> +			return offset;
> +
> +		default:
> +			break;
> +		}
> +	} while (tag != FDT_END);
> +
> +	return (nextoffset < 0) ? nextoffset : -FDT_ERR_NOTFOUND;
> +}
> +
>  int fdt_next_node(const void *fdt, int offset, int *depth)
>  {
>  	int nextoffset = 0;
>  	uint32_t tag;
>  
>  	if (offset >= 0)
> -		if ((nextoffset = fdt_check_node_offset_(fdt, offset)) < 0)
> +		if ((nextoffset = fdt_check_node_offset_(fdt, &offset)) < 0)
>  			return nextoffset;
>  
>  	do {
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 11f2e2ee..ec8af835 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -281,7 +281,7 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>  		while (*p == '/') {
>  			p++;
>  			if (p == end)
> -				return offset;
> +				goto terminate;
>  		}
>  		q = memchr(p, '/', end - p);
>  		if (! q)
> @@ -294,7 +294,13 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>  		p = q;
>  	}
>  
> -	return offset;
> +terminate:
> +        /*
> +         * Avoid returning offset 0 or the real root offset.
> +         * Be sure to return one and only one offset for the root node, its
> +         * real offset.
> +         */
> +	return offset ? offset : fdt_root_offset(fdt);
>  }
>  
>  int fdt_path_offset(const void *fdt, const char *path)
> @@ -304,14 +310,17 @@ int fdt_path_offset(const void *fdt, const char *path)
>  
>  const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>  {
> -	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
> +	const struct fdt_node_header *nh;
>  	const char *nameptr;
>  	int err;
>  
> -	if (!can_assume(VALID_DTB) && (((err = fdt_ro_probe_(fdt)) < 0)
> -	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)))
> -			goto fail;
> +	if (!can_assume(VALID_DTB) && (err = fdt_ro_probe_(fdt)) < 0)
> +		goto fail;
> +
> +	if ((err = fdt_check_node_offset_(fdt, &nodeoffset)) < 0)
> +		goto fail;
>  
> +	nh = fdt_offset_ptr_(fdt, nodeoffset);
>  	nameptr = nh->name;
>  
>  	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
> @@ -344,7 +353,7 @@ int fdt_first_property_offset(const void *fdt, int nodeoffset)
>  {
>  	int offset;
>  
> -	if ((offset = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> +	if ((offset = fdt_check_node_offset_(fdt, &nodeoffset)) < 0)
>  		return offset;
>  
>  	return nextprop_(fdt, offset);
> @@ -574,6 +583,7 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
>  {
>  	int pdepth = 0, p = 0;
>  	int offset, depth, namelen;
> +	int root_offset;
>  	const char *name;
>  
>  	FDT_RO_PROBE(fdt);
> @@ -581,7 +591,14 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
>  	if (buflen < 2)
>  		return -FDT_ERR_NOSPACE;
>  
> -	for (offset = 0, depth = 0;
> +	root_offset = fdt_root_offset(fdt);
> +	if (root_offset < 0)
> +		return root_offset;
> +
> +	if (!nodeoffset)
> +		nodeoffset = root_offset;
> +
> +	for (offset = root_offset, depth = 0;
>  	     (offset >= 0) && (offset <= nodeoffset);
>  	     offset = fdt_next_node(fdt, offset, &depth)) {
>  		while (pdepth > depth) {
> @@ -627,13 +644,21 @@ int fdt_supernode_atdepth_offset(const void *fdt, int nodeoffset,
>  {
>  	int offset, depth;
>  	int supernodeoffset = -FDT_ERR_INTERNAL;
> +	int root_offset;
>  
>  	FDT_RO_PROBE(fdt);
>  
>  	if (supernodedepth < 0)
>  		return -FDT_ERR_NOTFOUND;
>  
> -	for (offset = 0, depth = 0;
> +	root_offset = fdt_root_offset(fdt);
> +	if (root_offset < 0)
> +		return root_offset;
> +
> +	if (!nodeoffset)
> +		nodeoffset = root_offset;
> +
> +	for (offset = root_offset, depth = 0;
>  	     (offset >= 0) && (offset <= nodeoffset);
>  	     offset = fdt_next_node(fdt, offset, &depth)) {
>  		if (depth == supernodedepth)
> @@ -663,12 +688,15 @@ int fdt_supernode_atdepth_offset(const void *fdt, int nodeoffset,
>  int fdt_node_depth(const void *fdt, int nodeoffset)
>  {
>  	int nodedepth;
> -	int err;
> +	int offset;
> +
> +	offset = fdt_supernode_atdepth_offset(fdt, nodeoffset, 0, &nodedepth);
> +	if (offset < 0)
> +		return offset;
> +
> +	if (!can_assume(LIBFDT_FLAWLESS) && offset != fdt_root_offset(fdt))
> +		return -FDT_ERR_INTERNAL;
>  
> -	err = fdt_supernode_atdepth_offset(fdt, nodeoffset, 0, &nodedepth);
> -	if (err)
> -		return (can_assume(LIBFDT_FLAWLESS) || err < 0) ? err :
> -			-FDT_ERR_INTERNAL;
>  	return nodedepth;
>  }
>  
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 850aafe4..a1a07f01 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -226,7 +226,7 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
>  	int err;
>  	int allocated;
>  
> -	if ((nextoffset = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> +	if ((nextoffset = fdt_check_node_offset_(fdt, &nodeoffset)) < 0)
>  		return nextoffset;
>  
>  	namestroff = fdt_find_add_string_(fdt, name, namelen, &allocated);
> @@ -377,6 +377,12 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset,
>  
>  	FDT_RW_PROBE(fdt);
>  
> +	if (!parentoffset) {
> +		parentoffset = fdt_root_offset(fdt);
> +		if (parentoffset < 0)
> +			return parentoffset;
> +	}
> +
>  	offset = fdt_subnode_offset_namelen(fdt, parentoffset, name, namelen);
>  	if (offset >= 0)
>  		return -FDT_ERR_EXISTS;
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index c69a18ed..7a1915a5 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -503,6 +503,19 @@ int fdt_num_mem_rsv(const void *fdt);
>   */
>  int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size);
>  
> +/**
> + * fdt_root_offset - Get the offset of the root node
> + * @fdt: pointer to the device tree blob
> + *
> + * The root node can be located after the offset 0. Indeed FDT_NOP tags can be
> + * present at offset 0. fdt_root_offset() takes care of those possible FDT_NOP
> + * tags.
> + *
> + * returns: offset of the root node or negative libfdt error value otherwise
> + */
> +int fdt_root_offset(const void *fdt);
> +
> +
>  /**
>   * fdt_subnode_offset_namelen - find a subnode based on substring
>   * @fdt: pointer to the device tree blob
> @@ -1025,7 +1038,7 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen);
>   * at a specific depth from the root (where the root itself has depth
>   * 0, its immediate subnodes depth 1 and so forth).  So
>   *	fdt_supernode_atdepth_offset(fdt, nodeoffset, 0, NULL);
> - * will always return 0, the offset of the root node.  If the node at
> + * will always return the offset of the root node.  If the node at
>   * nodeoffset has depth D, then:
>   *	fdt_supernode_atdepth_offset(fdt, nodeoffset, D, NULL);
>   * will return nodeoffset itself.
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index 0e103caf..d5072279 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -20,7 +20,7 @@ int32_t fdt_ro_probe_(const void *fdt);
>  		}							\
>  	}
>  
> -int fdt_check_node_offset_(const void *fdt, int offset);
> +int fdt_check_node_offset_(const void *fdt, int *offset);
>  int fdt_check_prop_offset_(const void *fdt, int offset);
>  
>  const char *fdt_find_string_len_(const char *strtab, int tabsize, const char *s,
> diff --git a/libfdt/version.lds b/libfdt/version.lds
> index cbfef546..d0b71669 100644
> --- a/libfdt/version.lds
> +++ b/libfdt/version.lds
> @@ -7,6 +7,7 @@ LIBFDT_1.2 {
>  		fdt_string;
>  		fdt_num_mem_rsv;
>  		fdt_get_mem_rsv;
> +		fdt_root_offset;
>  		fdt_subnode_offset_namelen;
>  		fdt_subnode_offset;
>  		fdt_path_offset_namelen;
> --- 8< ---
> 
> 
> I hope all of those details will help to move forward.
> 
> Best regards,
> Hervé
> 

-- 
David Gibson (he or they)	| 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

Attachment: signature.asc
Description: PGP signature