Re: [PATCH v2 02/10] libfdt: Don't assume that a FDT_BEGIN_NODE tag is available at offset 0

[Herve Codina]

Hi David,

On Thu, 18 Jun 2026 20:13:57 +1000
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:

> On Thu, Apr 09, 2026 at 01:54:18PM +0200, Herve Codina wrote:
> > In several places, libfdt assumes that a FDT_BEGIN_NODE tag is present
> > at the offset 0 of the structure block.
> > 
> > This assumption is not correct. Indeed, a FDT_NOP can be present at the
> > offset 0 and this is a legit case.
> > 
> > fdt_first_node() has been introduced recently to get the offset of the
> > first node (first FDT_BEGIN_NODE) in a fdt blob.
> > 
> > Use this function to get the first node offset instead of looking for
> > this node at offset 0.
> > 
> > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>  
> 
> The problem is real, of course.  But this approach to solving it with
> a special case just for the root node is really ugly.
> 
> Granted, it's a problem of my own making - I chose not to create an
> fdt_root_offset() function in the first place, instead making it part
> of the API that offset 0 means the root node.  Nonetheless, here we
> are and the question is whether we can do better.
> 
> # Straightforward things first
> 
>  - This patch should be folded with 1/10, they're both harder to
>    understand without the context of the other.

Ok, I will squash, no problem.

> 
>  - If it must exist, the function should be fdt_root_offset(), not
>    fdt_first_node(), for at least three reasons:
>     * "first" in what sense?
>     * "first" amongst what set of nodes?
>     * We have a strong convention to always explicitly say "offset",
>       not just referring to offset values as "node" or "property".
>       This is deliberate: it's an attempt to discourage the otherwise
>       likely misunderstanding that a function getting a "node" gives
>       you some sort of persistent handle.  "offset" makes it clearer
>       that the value will no longer be valid after a modification to
>       the tree.

Make sense. I will rename to fdt_root_offset()

> 
>  - The situation described is subtle enough that this *really* needs a
>    testcase.  It shouldn't be that hard: change the existing
>    'nopulate' test tool to add an FDT_NOP before the first tag, not
>    just after

Yes, will add a test.


> 
> # Is FDT_NOP before the root node actually legitimate?
> 
> Arguably the simplest solution here would be to explicitly ban this.
> Yes, it would be a slightly odd restriction in the spec.  However,
> avoiding the mess in the library might be worth it.  Note that this
> situation can never arise from fdt_nop_node(), unless you apply it to
> the root node, in which case there's no tree left.

We tried to have something robust for future addition (structured tags).
Maybe a future tag will be nopified by some future tools before being
processed by libfdt.

IMHO, we should have support for FDT_NOP before the root node.

> 
> # Less special casery
> 
> Even if we accept the need for FDT_NOP before the root node, I think
> we can do better.  The below implements this as a special case, just
> for offset 0.  Instead, we could allow all node operations on a
> FDT_NOP offset, automatically advancing to the next FDT_BEGIN_NODE
> tag.  We may be able to do that in check_node_offset_() minimising
> code duplication.
> 

IHMO, check_node_offset_() should only check that the given offset is a
node and not trying to find the next node available after possible FDT_NOP.
Got the feeling that having this kind of search in check_node_offset_() is
error prone.

I am not sure that a lot of code duplication will be present. On some entry
points, we have this kind of code:
   --- 8< ---
   if (offset == 0) {
	offset = fdt_root_offset(fdt);
	if (offset < 0)
		return offset;
   }
   --- 8< ---

It has the benefit to keep things clear and is needed only on some entry
points (API function). Internal function should receive an offset pointing
to a node. For those internal function check_node_offset_() should not
automatically skip FDT_NOP tags but should really return an error if such a
tag is encountered.

For offsets other than offset 0, FDT_NOP is handled without any extra cost in
current code implementation.

Best regards,
Hervé