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

[David Gibson]

On Mon, Aug 31, 2026 at 02:01:19PM +0200, Herve Codina wrote:
> Hi David,
> 
> On Sun, 30 Aug 2026 13:21:06 +1000
> David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> ...
> 
> > > -int fdt_next_node(const void *fdt, int offset, int *depth)
> > > +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;  
> > 
> > This should be BADSTRUCTURE rather than NOTFOUND: a dtb without a root
> > node is not validly constructed.  (This could matter quite a lot if
> > this error gets propagated up a call chain - a NOTFOUND is usually
> > non-fatal, but BADSTRUCTURE means there's basically nothing that can
> > usefully be done with the dtb, which the caller needs to know as soon
> > as possible).
> 
> Now yes, a dtb without a root node is an invalid dtb but soon with addon this
> will be allowed [1].

Ah, ok.  I haven't looked at the addon stuff yet.  I do intend to, but
it will certainly take a while (70+ patches!).

So, I'm guessing here, but it still seems odd to me that an addon with
no BEGIN_NODE tags at all could be useful.

> addon dtbs will be clearly identified (the dt_flags header field has
> FDT_FLAG_ADDON set for addon dtbs) and so what do you think if the error code
> returned depends on this flag.
> 
> NOTFOUND in case of addon and BADSTRUCTURE otherwise.

I'd need to look at the details of addons to figure out if this makes
sense but I'm not opposed to the approach in principle.

> If you are ok with that, I will update this current patch to return BADSTRUCTURE
> in all case. Indeed, addon are not yet available.

Right, regardless of where we go with addons eventually, I think it's
preferable to return BADSTRUCTURE for now.  That can be changed once
addons are actually implemented.

> 
> [1] https://lore.kernel.org/all/20260826094950.1088288-54-herve.codina@xxxxxxxxxxx/
> 
> > 
> > > +}
> > > +
> > > +int fdt_next_node(const void *fdt, int offset, int *depth)
> > > +{
> > > +	int nextoffset = offset;  
> > 
> > This initialiser should be omitted, since it is now overwritten in
> > every possible case.
> 
> Yes indeed, will be updated in the next iteration.
> 
> > 
> > >  	uint32_t tag;
> > >  
> > > +	/*
> > > +	 * Get the root node if asked for next node from the root node
> > > +	 * (offset == 0) or if the given offset is not valid (negative).
> > > +	 */
> > > +	if (offset <= 0) {
> > > +		nextoffset = fdt_root_offset(fdt);
> > > +		if (nextoffset < 0)
> > > +			return nextoffset;
> > > +	}
> > > +  
> > 
> > The various changes you've made look correct, but I don't love the
> > fact that it requires nearly every function which takes a node offset
> > to be altered non-trivially.  As well as making for a large diff, it
> > strikes me as fragile - a bit of logic that could easily be forgotten
> > on a new function.
> > 
> > I think we want to move the offset 0 handling into a common helper.
> > Logically that should be fdt_check_node_offset_(), since that's the
> > standard way of validating a node offset parameter.  As you've pointed
> > out, that doesn't work with the current signature/semantics of
> > fdt_check_node_offset_() - but it's a strictly internal function, so
> > we can alter its signature freely.
> 
> Ok, you insist on this point. I will updade fdt_check_node_offset_() to
> handle the offset 0 case.

Great.

> 
> > 
> > >  	if (offset >= 0)
> > > -		if ((nextoffset = fdt_check_node_offset_(fdt, offset)) < 0)
> > > +		if ((nextoffset = fdt_check_node_offset_(fdt, nextoffset)) < 0)
> > >  			return nextoffset;
> > >  
> > >  	do {
> > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > index 11f2e2ee..856c62f1 100644
> > > --- a/libfdt/fdt_ro.c
> > > +++ b/libfdt/fdt_ro.c
> > > @@ -231,6 +231,12 @@ int fdt_subnode_offset_namelen(const void *fdt, int offset,
> > >  
> > >  	FDT_RO_PROBE(fdt);
> > >  
> > > +	if (!offset) {
> > > +		offset = fdt_root_offset(fdt);
> > > +		if (offset < 0)
> > > +			return offset;
> > > +	}
> > > +
> > >  	for (depth = 0;
> > >  	     (offset >= 0) && (depth >= 0);
> > >  	     offset = fdt_next_node(fdt, offset, &depth))
> > > @@ -253,13 +259,17 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
> > >  {
> > >  	const char *end = path + namelen;
> > >  	const char *p = path;
> > > -	int offset = 0;
> > > +	int offset;
> > >  
> > >  	FDT_RO_PROBE(fdt);
> > >  
> > >  	if (!can_assume(VALID_INPUT) && namelen <= 0)
> > >  		return -FDT_ERR_BADPATH;
> > >  
> > > +	offset = fdt_root_offset(fdt);
> > > +	if (offset < 0)
> > > +		return offset;
> > > +
> > >  	/* see if we have an alias */
> > >  	if (*path != '/') {
> > >  		const char *q = memchr(path, '/', end - p);
> > > @@ -304,14 +314,24 @@ 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 (!nodeoffset) {
> > > +		nodeoffset = fdt_root_offset(fdt);
> > > +		if (nodeoffset < 0) {
> > > +			err = nodeoffset;
> > > +			goto fail;
> > > +		}
> > > +	}
> > > +
> > > +
> > >  	if (!can_assume(VALID_DTB) && (((err = fdt_ro_probe_(fdt)) < 0)
> > >  	    || ((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,6 +364,12 @@ int fdt_first_property_offset(const void *fdt, int nodeoffset)
> > >  {
> > >  	int offset;
> > >  
> > > +	if (!nodeoffset) {
> > > +		nodeoffset = fdt_root_offset(fdt);
> > > +		if (nodeoffset < 0)
> > > +			return nodeoffset;
> > > +	}
> > > +
> > >  	if ((offset = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > >  		return offset;
> > >  
> > > @@ -581,7 +607,7 @@ 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;
> > > +	for (offset = fdt_root_offset(fdt), depth = 0;
> > >  	     (offset >= 0) && (offset <= nodeoffset);
> > >  	     offset = fdt_next_node(fdt, offset, &depth)) {
> > >  		while (pdepth > depth) {
> > > @@ -619,7 +645,7 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
> > >  	else if (offset == -FDT_ERR_BADOFFSET)
> > >  		return -FDT_ERR_BADSTRUCTURE;
> > >  
> > > -	return offset; /* error from fdt_next_node() */
> > > +	return offset; /* error from fdt_next_node() or fdt_root_offset() */
> > >  }
> > >  
> > >  int fdt_supernode_atdepth_offset(const void *fdt, int nodeoffset,
> > > @@ -627,13 +653,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;  
> > 
> > Do you need this special casing? Won't the fact you've update
> > fdt_next_node() to handle the offset 0 case be enough?
> 
> The full loop is the following:
> --- 8< ---
> 	for (offset = root_offset, depth = 0;
> 	     (offset >= 0) && (offset <= nodeoffset);
> 	     offset = fdt_next_node(fdt, offset, &depth)) {
> 		if (depth == supernodedepth)
> 			supernodeoffset = offset;
> 
> 		if (offset == nodeoffset) {
> 			if (nodedepth)
> 				*nodedepth = depth;
> 
> 			if (supernodedepth > depth)
> 				return -FDT_ERR_NOTFOUND;
> 			else
> 				return supernodeoffset;
> 		}
> 	}
> --- 8< ---
> 
> The test 'offset == nodeoffset' is the problematic one. nodeoffset
> is the parameter passed to the function.
> 
> I have chosen to avoid offset 0 for the root node and I have updated 'nodeoffset'
> previously if it is 0.
> 
> Even if fdt_next_node() updates 'offset' if it is 0, 'nodeoffset' has also to be
> update if it is 0. Indeed, 'offset' will be updated from 0 to the real root node
> offset. In all case to have the test be correct, 'nodeoffset' should be updated
> to the real root node offset if it is 0.

> Having fdt_next_node() updating 'offset' if it is 0 will not handle all case.
> When offsets comparison is done, both offsets should consider the real offset of
> the root node instead of the 0 value.

Ah, right.  I missed the fact it was updating the nodeoffset
parameter, rather than the working/starting offset.

> Also the fdt_supernode_atdepth_offset() can be called with 'nodeoffset' set to the
> real root node offset instead of 0. Indeed, fdt_root_offset() is available (and
> needed).

Theoretically we could avoid explicitly looking at the root offset for
'offset', by starting 'offset' negative and moving the fdt_next_node()
to the start of the loop body instead of the end.  That would handle
the nodeoffset == root_offset case, not not the nodeoffset == 0 case.
Arguably we could disallow the later - finding the non-existent
supernode of something we know at compile time to be the root node
isn't very useful - if it only turned out to be the root node at
runtime, I'd expect it to have come from another function, which
should give us root_offset rather than 0.

But.. it's certainly safer to keep it working whether passed 0 or the
real root offset.  There are other ways we could do it, but I think
they'd turn out equally inelegant.

Ok, makes sense to me.

> My feeling was that it is less error prone to have offset 0 converted to the real
> root node offset as soon as possible and then consider only the real offset of the
> root node instead of a mix between the real offset and the offset 0.

That's fair.

> I think that "for (offset = root_offset, ..." will still be needed.
> 
> 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