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: 7861 bytes --]

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.

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.


> 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
for the interface compatibility is whether they're unknown to the
caller, which we can't know.  So really this is checking for
structured tags specifically.

> +{
> +	switch (tag) {
> +	case FDT_BEGIN_NODE:
> +	case FDT_END_NODE:
> +	case FDT_PROP:
> +	case FDT_NOP:
> +	case FDT_END:
> +		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.

> +{
> +	uint32_t tag;
> +	bool unknown = false;
> +
> +	/* Retrieve next tag */
> +	tag = fdt_next_tag_all(fdt, startoffset, nextoffset);
> +	if (tag == FDT_END)
> +		goto end;
> +
> +	if (fdt_tag_is_unknown(tag)) {
> +		unknown = true;
> +		/* Use a known tag that should be skipped by the caller */
> +		tag = FDT_NOP;
> +	}
> +end:
> +	if (is_unknown)
> +		*is_unknown = unknown;
> +	return tag;
> +}
> +
> +uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> +{
> +	return fdt_next_tag_(fdt, startoffset, nextoffset, NULL);
> +}
> +
>  int fdt_check_node_offset_(const void *fdt, int offset)
>  {
>  	if (!can_assume(VALID_INPUT)
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index 0e103caf..f2e30ce8 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -20,6 +20,9 @@ int32_t fdt_ro_probe_(const void *fdt);
>  		}							\
>  	}
>  
> +uint32_t fdt_next_tag_(const void *fdt, int startoffset, int *nextoffset,
> +		       bool *is_unknown);



> +
>  int fdt_check_node_offset_(const void *fdt, int offset);
>  int fdt_check_prop_offset_(const void *fdt, int offset);
>  
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 8fc23cb7..225c22f8 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -577,11 +577,12 @@ libfdt_tests () {
>      run_test dtbs_equal_ordered cell-overflow.test.dtb cell-overflow-results.test.dtb
>  
>      # check full tests
> -    for good in test_tree1.dtb; do
> +    for good in test_tree1.dtb unknown_tags_can_skip.dtb; do
>  	run_test check_full $good
>      done
>      for bad in truncated_property.dtb truncated_string.dtb \
> -		truncated_memrsv.dtb two_roots.dtb named_root.dtb; do
> +		truncated_memrsv.dtb two_roots.dtb named_root.dtb \
> +		unknown_tags_no_skip.dtb; do
>  	run_test check_full -n $bad
>      done
>  }
> @@ -962,6 +963,10 @@ fdtget_tests () {
>      run_fdtget_test "<the dead silence>" -tx \
>  	-d "<the dead silence>" $dtb /randomnode doctor-who
>      run_fdtget_test "<blink>" -tx -d "<blink>" $dtb /memory doctor-who
> +
> +    # test with unknown tags involved
> +    run_fdtget_test "25601 25602" unknown_tags_can_skip.dtb /subnode1 prop-int
> +    run_wrap_error_test $DTGET unknown_tags_no_skip.dtb /subnode1 prop-int
>  }
>  
>  fdtput_tests () {
> -- 
> 2.55.0
> 
> 

-- 
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 --]