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

[Herve Codina <herve.codina@bootlin.com>]

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));


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.

> 
> 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().

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.

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. 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 am not fully convinced by this option.

c) lib version
   I disagree on my side

d) Other idea
   Of course, I am still open.

> 
> 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  libfdt/fdt.c             | 75 ++++++++++++++++++++++++++++++++++++++--
> >  libfdt/libfdt_internal.h |  3 ++
> >  tests/run_tests.sh       |  9 +++--
> >  3 files changed, 83 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > index eb803e8a..506e0dd3 100644
> > --- a/libfdt/fdt.c
> > +++ b/libfdt/fdt.c
> > @@ -167,7 +167,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> >  	return fdt_offset_ptr_(fdt, offset);
> >  }
> >  
> > -uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > +static uint32_t fdt_next_tag_all(const void *fdt, int startoffset, int *nextoffset)
> >  {
> >  	const fdt32_t *tagp, *lenp;
> >  	uint32_t tag, len, sum;
> > @@ -218,7 +218,37 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> >  		break;
> >  
> >  	default:
> > -		return FDT_END;
> > +		if (!(tag & FDT_TAG_STRUCTURED) || !(tag & FDT_TAG_SKIP_SAFE))
> > +			return FDT_END;
> > +
> > +		switch (tag & FDT_TAG_DATA_MASK) {
> > +		case FDT_TAG_DATA_NONE:
> > +			break;
> > +		case FDT_TAG_DATA_1CELL:
> > +			offset += FDT_CELLSIZE;
> > +			break;
> > +		case FDT_TAG_DATA_2CELLS:
> > +			offset += 2 * FDT_CELLSIZE;
> > +			break;
> > +		case FDT_TAG_DATA_VARLEN:
> > +			/* Get the length */
> > +			lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> > +			if (!can_assume(VALID_DTB) && !lenp)
> > +				return FDT_END; /* premature end */
> > +			len = fdt32_to_cpu(*lenp);
> > +			/*
> > +			 * Skip the cell encoding the length and the
> > +			 * following length bytes
> > +			 */
> > +			len += sizeof(*lenp);
> > +			sum = len + offset;
> > +			if (!can_assume(VALID_DTB) &&
> > +			    (sum >= INT_MAX || sum < (uint32_t) offset))
> > +				return FDT_END; /* premature end */
> > +
> > +			offset += len;
> > +			break;
> > +		}
> >  	}
> >  
> >  	if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > @@ -228,6 +258,47 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> >  	return tag;
> >  }
> >  
> > +static bool fdt_tag_is_unknown(uint32_t tag)  
> 
> Regardless of what approach we decide to take, "unknown" is a
> misleading term here.  As we add support for new metadata tags to
> libfdt, they will no longer be unknown to libfdt.  But what matters

Yes, metadata tags once supported in libfdt will not be considered as "unknown"
tags.

An unknow tag is a tag libfdt don't know about except:
- weither it can be skip or not,
- the data length related to this tag.
- Is relation ship with other objects:
   - Global to the whole dtb if found out of any nodes
   - Related to a node if found right after FDT_BEGIN_NODE
   - Related to a poperty if found right after FDT_PROP

> for the interface compatibility is whether they're unknown to the
> caller, which we can't know.  So really this is checking for

If a caller need to handle this unknown tag, it has to use a libfdt which
support this tag.

> structured tags specifically.
> 
> > +{
> > +	switch (tag) {
> > +	case FDT_BEGIN_NODE:
> > +	case FDT_END_NODE:
> > +	case FDT_PROP:
> > +	case FDT_NOP:
> > +	case FDT_END:

This list will grow as soon as new tags are defined and supported in libfdt.

See all tags added by the addon series:
https://github.com/bootlin/dtc/blob/c68038e0ff4cde5de37a21419df8a082032ef994/libfdt/fdt.c#L435

> > +		return false;
> > +	default:
> > +		break;
> > +	}
> > +	return true;
> > +}
> > +
> > +uint32_t fdt_next_tag_(const void *fdt, int startoffset, int *nextoffset, bool *is_unknown)  
> 
> 
> Nothing in this patch uses the is_unknown parameter.  Presumably
> something later does, but it's not clear to me what it's useful for.

Yes later in [PATCH v3 14/15] libfdt: Handle unknown tags on dtb modifications
In this patch, libfdt/fdt_rw.c, fdt_prop_remove_unknown_tags().
Code also in git repo [0].

[0] https://github.com/bootlin/dtc/blob/2653bb9cd4414780c863051467b5e77eea6e5bdd/libfdt/fdt_rw.c#L211

Best regards,
Hervé