Re: [PATCH v3 08/15] flattree: Handle unknown tags

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

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

On Wed, Aug 26, 2026 at 10:31:39AM +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.
> 
> Handle those structured tag.
> 
> 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>
> ---
>  flattree.c                                 | 65 ++++++++++++++++++++--
>  tests/run_tests.sh                         |  5 ++
>  tests/unknown_tags_can_skip.dtb.dts.expect | 19 +++++++
>  3 files changed, 84 insertions(+), 5 deletions(-)
>  create mode 100644 tests/unknown_tags_can_skip.dtb.dts.expect
> 
> diff --git a/flattree.c b/flattree.c
> index f3b698c1..88dbfa7e 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -579,7 +579,8 @@ static void flat_read_chunk(struct inbuf *inb, void *p, int len)
>  	if ((inb->ptr + len) > inb->limit)
>  		die("Premature end of data parsing flat device tree\n");
>  
> -	memcpy(p, inb->ptr, len);
> +	if (p)
> +		memcpy(p, inb->ptr, len);
>  
>  	inb->ptr += len;
>  }
> @@ -604,6 +605,61 @@ static void flat_realign(struct inbuf *inb, int align)
>  		die("Premature end of data parsing flat device tree\n");
>  }
>  
> +static bool flat_skip_unknown_tag(struct inbuf *inb, uint32_t tag)
> +{
> +	uint32_t lng;
> +
> +	if (!(tag & FDT_TAG_STRUCTURED) || !(tag & FDT_TAG_SKIP_SAFE))
> +		return false;
> +
> +	switch (tag & FDT_TAG_DATA_MASK) {
> +	case FDT_TAG_DATA_NONE:
> +		break;
> +
> +	case FDT_TAG_DATA_1CELL:
> +		flat_read_word(inb);
> +		break;
> +
> +	case FDT_TAG_DATA_2CELLS:
> +		flat_read_word(inb);
> +		flat_read_word(inb);
> +		break;
> +
> +	case FDT_TAG_DATA_VARLEN:
> +		/* Get the length */
> +		lng = flat_read_word(inb);

I think it would be more natural to get the length as a single value,
then have a common flat_read_chunk() and flat_realign() to consume it.
That's for two reasons:
 * Assuming we keep this length encoding, getting the final tag size
   seems like it would make a useful helper function anyway.
 * Using flat_read_word() is misleading - it implies it's integer data
   where endianness matters.  In this case it's not - it's just some
   bytes we're skipping over, we don't know the internal structure.

> +
> +		/* Skip the following length bytes */
> +		flat_read_chunk(inb, NULL, lng);
> +
> +		flat_realign(inb, sizeof(uint32_t));
> +		break;
> +	}
> +
> +	return true;
> +}
> +
> +static uint32_t flat_read_tag(struct inbuf *inb)
> +{
> +	uint32_t tag;
> +
> +	do {
> +		tag = flat_read_word(inb);
> +		switch (tag) {
> +		case FDT_BEGIN_NODE:
> +		case FDT_END_NODE:
> +		case FDT_PROP:
> +		case FDT_NOP:
> +		case FDT_END:
> +			return tag;
> +		default:
> +			break;
> +		}
> +	} while (flat_skip_unknown_tag(inb, tag));

Having this as a separate function seems odd to me...

> +	die("Cannot skip unknown tag 0x%08x\n", tag);
> +}
> +
>  static const char *flat_read_string(struct inbuf *inb)
>  {
>  	int len = 0;
> @@ -750,7 +806,7 @@ static struct node *unflatten_tree(struct inbuf *dtbuf,
>  		struct property *prop;
>  		struct node *child;
>  
> -		val = flat_read_word(dtbuf);
> +		val = flat_read_tag(dtbuf);
>  		switch (val) {


.. rather than having handling unknown tags as part of the default:
case here.

>  		case FDT_PROP:
>  			if (node->children)
> @@ -905,14 +961,13 @@ struct dt_info *dt_from_blob(const char *fname)
>  
>  	reservelist = flat_read_mem_reserve(&memresvbuf);
>  
> -	val = flat_read_word(&dtbuf);
> -
> +	val = flat_read_tag(&dtbuf);
>  	if (val != FDT_BEGIN_NODE)
>  		die("Device tree blob doesn't begin with FDT_BEGIN_NODE (begins with 0x%08x)\n", val);

Hmm.. doesn't this already need to be fixed to handle NOP tags before
the root node?  Logically that change would go before this one.

>  
>  	tree = unflatten_tree(&dtbuf, &strbuf, "", flags);
>  
> -	val = flat_read_word(&dtbuf);
> +	val = flat_read_tag(&dtbuf);
>  	if (val != FDT_END)
>  		die("Device tree blob doesn't end with FDT_END\n");

Likewise here for that matter, a NOP should be valid between the last
FDT_END_NODE and the FDT_END.

> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index f3647e63..8fc23cb7 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -882,6 +882,11 @@ dtc_tests () {
>  
>      # Tests for overlay/plugin generation
>      dtc_overlay_tests
> +
> +    # Tests with "unknown tags"
> +    run_dtc_test -I dtb -O dts -o unknown_tags_can_skip.dtb.dts unknown_tags_can_skip.dtb
> +    base_run_test check_diff unknown_tags_can_skip.dtb.dts "$SRCDIR/unknown_tags_can_skip.dtb.dts.expect"

It's best to avoid tests based on -O dts output unless we're
explicitly checking -O dts behaviour: because there are multiple ways
to format property values, the exact output isn't really guaranteed.

What I'd suggest instead is to adjust treegen to generate two dtbs
that are identical _except_ for the skippable tag.  Then you can use
dtc -I dtb -O dtb, and compare the dtc output (which should strip the
tag) against the dtb which was constructed without it in the first
place.

Or, rather than explicitly creating two new trees, you could make your
skippable tag example identical to test_tree1, except for the
additional tag, and re-use one of the other instances of test_tree1 as
the "tagless" version.


> +    run_wrap_error_test $DTC -I dtb -O dts -o unknown_tags_no_skip.dtb.dts unknown_tags_no_skip.dtb
>  }
>  
>  cmp_tests () {
> diff --git a/tests/unknown_tags_can_skip.dtb.dts.expect b/tests/unknown_tags_can_skip.dtb.dts.expect
> new file mode 100644
> index 00000000..2194025b
> --- /dev/null
> +++ b/tests/unknown_tags_can_skip.dtb.dts.expect
> @@ -0,0 +1,19 @@
> +/dts-v1/;
> +
> +/ {
> +	prop-int = <0x3201>;
> +	prop-str = "abcd";
> +
> +	subnode1 {
> +		prop-int = <0x6401 0x6402>;
> +	};
> +
> +	subnode2 {
> +		prop-int1 = <0x64020 0x64021>;
> +		prop-int2 = <0x32022>;
> +
> +		subsubnode {
> +			prop-bool;
> +		};
> +	};
> +};
> -- 
> 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 --]