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

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

Hi David,

On Sat, 19 Sep 2026 14:46:05 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

...

> 
> > 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 did my homework and I think I was wrong. fdt_next_tag() can return all tags
without adding too much complexity in current code.

Here is the diff to consider that fully replaces this current patch.

You can note that I've added the FDT_TAG_IS_SKIPPABLE() macro. This macro
should be added in a previous commit introducing structured tag (or skippable
if renamed). According to discussion we've already had related to "structure"
tag format, the FDT_TAG_IS_SKIPPABLE() macro content will not be that one but
you've got the idea of its usage in the diff.

--- 8< ---
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index eb803e8a..e426414b 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -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))
@@ -303,6 +333,7 @@ int fdt_next_node(const void *fdt, int offset, int *depth)
 		switch (tag) {
 		case FDT_PROP:
 		case FDT_NOP:
+		default: /* All Skippable tags */
 			break;
 
 		case FDT_BEGIN_NODE:
diff --git a/libfdt/fdt.h b/libfdt/fdt.h
index f41a355f..55138261 100644
--- a/libfdt/fdt.h
+++ b/libfdt/fdt.h
@@ -73,6 +73,8 @@ struct fdt_property {
 #define FDT_TAG_CAN_SKIP(tag_data, tag_id) \
 		(FDT_TAG_STRUCTURED | FDT_TAG_SKIP_SAFE | tag_data | tag_id)
 
+#define FDT_TAG_IS_SKIPPABLE(_tag) (((_tag) & (FDT_TAG_STRUCTURED | FDT_TAG_SKIP_SAFE)) == (FDT_TAG_STRUCTURED | FDT_TAG_SKIP_SAFE))
+
 /* Tests reserved tags */
 #define FDT_TEST_NONE_CAN_SKIP		FDT_TAG_CAN_SKIP(FDT_TAG_DATA_NONE, 0)
 #define FDT_TEST_1CELL_CAN_SKIP		FDT_TAG_CAN_SKIP(FDT_TAG_DATA_1CELL, 0)
diff --git a/libfdt/fdt_check.c b/libfdt/fdt_check.c
index 2fd5b61d..8bd9b252 100644
--- a/libfdt/fdt_check.c
+++ b/libfdt/fdt_check.c
@@ -45,7 +45,8 @@ int fdt_check_full(const void *fdt, size_t bufsize)
 			return nextoffset;
 
 		/* If we see two root nodes, something is wrong */
-		if (expect_end && tag != FDT_END && tag != FDT_NOP)
+		if (expect_end && tag != FDT_END && tag != FDT_NOP &&
+			!FDT_TAG_IS_SKIPPABLE(tag))
 			return -FDT_ERR_BADSTRUCTURE;
 
 		switch (tag) {
@@ -92,7 +93,8 @@ int fdt_check_full(const void *fdt, size_t bufsize)
 			break;
 
 		default:
-			return -FDT_ERR_INTERNAL;
+			/* Skippable tags -> Skip them */
+			break;
 		}
 	}
 }
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 856c62f1..3cc0435c 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -219,7 +219,7 @@ static int nextprop_(const void *fdt, int offset)
 			return offset;
 		}
 		offset = nextoffset;
-	} while (tag == FDT_NOP);
+	} while (tag == FDT_NOP || FDT_TAG_IS_SKIPPABLE(tag));
 
 	return -FDT_ERR_NOTFOUND;
 }
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 () {
--- 8< ---

Also I've modified tests/dtbs_equal_ordered.c but I am not sure that the
modification I did is a modification you will accept.

We could just ignore skippable tags but I choose to test data when such an
skippable tag is encoutered.

This leads to the following modification:
--- 8< ---
--- a/tests/dtbs_equal_ordered.c
+++ b/tests/dtbs_equal_ordered.c
@@ -71,6 +71,7 @@ static void compare_structure(const void *fdt1, const void *fdt2)
 	int err;
 	const struct fdt_property *prop1, *prop2;
 	int len1, len2;
+	const void *raw1, *raw2;
 
 	while (1) {
 		do {
@@ -102,6 +103,9 @@ static void compare_structure(const void *fdt1, const void *fdt2)
 				     name1, name2, offset1, offset2);
 			break;
 
+		case FDT_END_NODE:
+			break;
+
 		case FDT_PROP:
 			prop1 = fdt_offset_ptr(fdt1, offset1, sizeof(*prop1));
 			if (!prop1)
@@ -128,6 +132,36 @@ static void compare_structure(const void *fdt1, const void *fdt2)
 
 		case FDT_END:
 			return;
+
+		default:
+			if (!FDT_TAG_IS_SKIPPABLE(tag1))
+				FAIL("Cannot check tag data at (%d, %d)",
+					offset1, offset2);
+
+			/*
+			 * Even if we don't known the meaning of data,
+			 * we can check that raw data are the same.
+			 */
+			len1 = nextoffset1 - offset1;
+			len2 = nextoffset2 - offset2;
+
+			if (len1 != len2) {
+				MISMATCH("tag data length mismatch at (%d, %d)",
+					offset1, offset2);
+			}
+
+			raw1 = fdt_offset_ptr(fdt1, offset1, len1);
+			if (!raw1)
+				FAIL("Could get fdt1 raw buffer at %d", offset1);
+
+			raw2 = fdt_offset_ptr(fdt2, offset2, len2);
+			if (!raw1)
+				FAIL("Could get fdt1 raw buffer at %d", offset2);
+
+			if (memcmp(raw1, raw2, len1) != 0)
+				MISMATCH("Tag data value mismatch at (%d, %d)",
+					 offset1, offset2);
+			break;
 		}
 	}
 }
--- 8< ---

With that done, my conclusion is: Yes we can return all tags in fdt_next_tag().
I think you will agree this conclusion.

For dtbs_equal_ordered.c, should we compare skippable tags and related data as
I did (diff above) or should we just skip them? what is your opinion?

For the next patch ("libfdt: Handle unknown tags on dtb modifications")
returning all tags from fdt_next_tag() has also some impacts but nothing more
complex than modification provided here. I can provide the full diff if you
want. Let me know.

Best regards,
Hervé