Re: [PATCH v3 09/15] libfdt: Handle unknown tags in fdt_next_tag()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9022 bytes --]

On Thu, Sep 17, 2026 at 07:28:14PM +0200, Herve Codina wrote:
> Hi David,
> 
> On Thu, 17 Sep 2026 19:36:06 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Sep 17, 2026 at 10:34:10AM +0200, Herve Codina wrote:
> > > Hi David,
> > > 
> > > On Wed, 16 Sep 2026 19:10:27 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Wed, Aug 26, 2026 at 10:31:40AM +0200, Herve Codina wrote:  
> > > > > The structured tag value definition introduced recently gives the
> > > > > ability to ignore unknown tags without any error when they are read.
> > > > > 
> > > > > libfdt uses fdt_next_tag() to get a tag.
> > > > > 
> > > > > Filtering out tags that should be ignored in fdt_next_tag() allows to
> > > > > have the filtering done globally and allows, in future releases, to have
> > > > > a central place to add new known tags that should not be filtered out.
> > > > > 
> > > > > An already known tag exists with the meaning of "just ignore". This tag
> > > > > is FDT_NOP. fdt_next_tag() callers already handle the FDT_NOP tag.
> > > > > 
> > > > > Avoid unneeded modification at callers side and use a fake FDT_NOP tag
> > > > > when an unknown tag that should be ignored is encountered.
> > > > > 
> > > > > Add also fdt_next_tag_() internal function for callers who need to know
> > > > > if the FDT_NOP tag returned is a real FDT_NOP or a fake FDT_NOP due to
> > > > > an unknown tag.    
> > > > 
> > > > I'm a bit unsure about this one.  If we were designing the libfdt
> > > > interface from scratch, I'd definitely say that fdt_next_tag() - as a
> > > > low level function - should return every tag, and it's up to the
> > > > caller to skip unknown ones.  fdt_next_tag() already provides
> > > > nextoffset making it fairly easy to do so.
> > > > 
> > > > But, of course, we're not building from scratch so we do need to
> > > > consider backwards compatibility.  Supplying a skipping and
> > > > non-skipping version as you have here is the obvious approach.
> > > > 
> > > > But I'm not entirely convinced it's the only or best approach.  A user
> > > > which calls fdt_next_tag() is, in a sense, already opting in to
> > > > low-level handling of the dtb.  They could well already be broken by
> > > > new tags (depending on how they handle the default case).  Arguably
> > > > it's reasonable to require them to be updated (once only) for this new
> > > > family of tags.  
> > > 
> > > I can go in that way. We will have same kind of sequence in almost all
> > > callers.
> > > 
> > >    do {
> > >       tag = fdt_next_tag()
> > >    } while (!fdt_tag_is_unknown(tag));  
> > 
> > Not really - users of fdt_next_tag() will typically already have a
> > switch on the tag type, so the unknown tag handling would be an
> > addition to the 'default' case, not requiring an extra loop.
> 
> Ok, those are to be update to use a switch on tag but that's not big deal
>  - fdt_check_node_offset_()
>  - fdt_check_prop_offset_()

These don't have a switch because they're asserting we're already on a
specific tag type.  They should still do that with structured tags:
these functions should be checking that we're exactly on an
FDT_BEGIN_NODE or FDT_PROP, not on a skippable tag that sits before
one of those.

Or to put it another way, if we did have fdt_next_tag()
vs. fdt_next_tag_all() these functions should be using
fdt_next_tag_all().

>  - fdt_add_subnode_namelen()

The logic here is placing the subnode after the parent's properties,
by moving past any FDT_PROP or FDT_NOP tags.  With skippable tags we
do need to skip those too.  Which we can express instead as skipping
until we find either an FDT_BEGIN_NODE (indicating the first child) or
an FDT_END_NODE (indicating the end of a currently childless parent).
So, no need for a new switch here.

>  - fdt_finish()

No need for changes here either - this already skips everything that's
not FDT_PROP, so it will do the right think for skippable tags.

> Some others like nextprop_() or fdt_next_node() mix a while loop and a
> switch/case. Some rework to be planned.

nextprop_() will need some rework, yes.

fdt_next_node() only barely: it already ignores FDT_PROP and FDT_NOP;
change that to 'default' and it will skip those and future structured
tags as well.

> > > We have to care about those "unknown_and_skippable" tags only when
> > > we modify a dtb. This is handled by a few functions.
> > > 
> > > The vast majority of callers of fdt_next_tag() just want to skip
> > > unknown tags.  
> > 
> > I'm not sure that's true of the (few) callers outside libfdt itself.
> 
> Well, today there is no "unknown" tags and existing callers works
> correctly.

Rather, any tags unknown by the caller are assumed fatal.

> Unknown tags are reported as FDT_NOP. It should be ok with existing callers.
> They should handle FDT_NOP.
> 
> > 
> > > > Another potential approach would be to actually use the symbol
> > > > versioning we have, but have som far only minimally exploited.  The
> > > > LIBFDT_1.2 version of fdt_next_tag() would skip tags that postdate it,
> > > > but we'd introduce a new version tag and the new default version of
> > > > fdt_next_tag() would return all tags.  That would of course require
> > > > figuring out how to do that with the verison script.  
> > > 
> > > Hum, I really don't want to have versioning in loop for fdt_next_tag().
> > 
> > I'm not sure what you mean by "in loop" here.
> 
> I don't want to add the versioning extra complexity in this context.
> I am sure we can find a solution for this "all tags vs only known tags"
> topic without versioning involved.

"known tags" is meaningless here - what matters is whether the tags
are known by the *caller*, which we don't know.

> > > Also, this could work in the dynamic lib (.so) case but not for static
> > > link and even less for libfdt code copy compiled in bootloaders or Linux
> > > kernel code.  
> > 
> > Are symbol versions entirely unsupported with static libraries?
> 
> I would say yes but maybe I am wrong.
> 
> https://sourceware.org/binutils/docs/ld/VERSION.html
> https://sourceware.org/binutils/docs/ld.html#index-_002d_002dversion_002dscript_003dversion_002dscriptfile

Ah, yes, I think you're right.  Ok, that's not going to work.
> 
> > Obviously they'd have to be resolved at (static) link time, not
> > runtime, but I don't see that it's impossible.
> > 
> > > fdt_next_tag() is a low level function accessible out of libfdt (API).
> > > If it doesn't skip unknown tags even for other libfdt internal function it
> > > will be a too lower function.  
> > 
> > I don't really follow what you're saying here.
> 
> Skipping unknown tag is a low level feature.
> I think fdt_next_tag() can handle this low level feature.

It really can't, because it doesn't know what tags the caller is aware
of.  It can only make a distinction between "older tags than
<arbitrary cutoff>" and "newer tags than <arbitrary cutoff>".

> Upper layer using fdt_next_tag() should not handle this tag skipping the
> general case and let fdt_next_tag() do this job.
> 
> Got the feeling that we are going to add complexity at upper layer to handle
> a feature that should be handled at low level.
> 
> > 
> > > Ok, we can have a wrapper on top of
> > > fdt_next_tag() to skip unknown tags.
> > > 
> > > fdt_next_tag() is a low level function but the rule related to skipping
> > > unknown tags is also a low level rule.
> > > 
> > > We have to choose:
> > > a) fdt_next_tag() as I proposed
> > >    Of course ok for me
> > > 
> > > b) fdt_next_tag() returns all tags -> Full handling at caller side
> > >    Ok, we can go in that direction but it will bring quite a lot of
> > >    code duplication.  
> > 
> > I'm not sure it's that bad, since fdt_next_tag() loops will typically
> > already have a 'default' case for irrelevant tags, which will
> > generally be handled the same as unknown tags.
> 
> Well as we did for "offset 0 vs real root node offset", I think the best way
> to known if it's bad or not is to try.

Agreed.

> I will not have time this week to propose something and I have a training
> next week.
> 
> If you're ok for a try, I will do it but not as quickly as I did for the
> "offset 0 vs real root node offset" topic.
> 
> > 
> > > I am not fully convinced by this option.
> > > 
> > > c) lib version
> > >    I disagree on my side  
> > 
> > Yeah, I'm not really convinced by this one either.
> 
> We are on the same page.
> 
> > 
> > > d) Other idea
> > >    Of course, I am still open.  
> > 
> > I haven't thought of any more ideas yet.
> > 
> 
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]