[PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[Paul Barker]

When applying an overlay fragment, we should take care not to overwrite
an existing phandle property of the target node as this could break
references to the target node elsewhere in the base dtb.

In addition to potentially breaking references within the resulting fdt,
if the overlay is built with symbols enabled (`-@` option to dtc) then
fdtoverlay will be unable to merge the overlay with a base dtb file.

A new test case is added to check how fdtoverlay handles this case.
Attempting to apply this test overlay without the fix in this patch
results in the following output:

    input  = tests/overlay_base_ref.test.dtb
    output = tests/overlay_overlay_ref.fdtoverlay.dtb
    overlay[0] = tests/overlay_overlay_ref.test.dtb

    Failed to apply 'tests/overlay_overlay_ref.test.dtb': FDT_ERR_NOTFOUND

In this test case the __overlay__ node in question does not explicitly
contain a phandle property in the dts file, the phandle is added during
compilation as it is referenced by another node within the overlay dts.

This failure occurs due to a sequence of events in the functions called
by fdt_overlay_apply():

1) In overlay_fixup_phandles(), the target of the overlay fragment is
   looked up and the target property is set to the phandle of the target
   node.

2) In overlay_merge(), the target node is looked up by phandle via
   overlay_get_target(). As the __overlay__ node in this test case
   itself has a phandle property, the phandle of the target node is
   modified.

3) In overlay_symbol_update(), the target node is again looked up by
   phandle via overlay_get_target(). But this time the target node
   cannot be found as its phandle property was modified.

The fix for this issue is to skip modification of the phandle property
of the target node in step (2) of the above sequence. If the target node
doesn't already contain a phandle property, we can add one without risk.

Signed-off-by: Paul Barker <pbarker-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 libfdt/fdt_overlay.c          |  2 ++
 tests/overlay_base_ref.dts    | 19 +++++++++++++++++++
 tests/overlay_overlay_ref.dts | 24 ++++++++++++++++++++++++
 tests/run_tests.sh            |  5 +++++
 4 files changed, 50 insertions(+)
 create mode 100644 tests/overlay_base_ref.dts
 create mode 100644 tests/overlay_overlay_ref.dts

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index d217e79..b3c217a 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -573,6 +573,8 @@ static int overlay_apply_node(void *fdt, int target,
 		if (prop_len < 0)
 			return prop_len;
 
+		if (!strcmp(name, "phandle") && fdt_getprop(fdt, target, name, NULL))
+			continue;
 		ret = fdt_setprop(fdt, target, name, prop, prop_len);
 		if (ret)
 			return ret;
diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
new file mode 100644
index 0000000..1fc02a2
--- /dev/null
+++ b/tests/overlay_base_ref.dts
@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+
+/ {
+	test: test-node {
+		test-int-property = <42>;
+	};
+
+	test-refs {
+		refs = <&test>;
+	};
+};
diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
new file mode 100644
index 0000000..a45c95d
--- /dev/null
+++ b/tests/overlay_overlay_ref.dts
@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+/ {
+	fragment@0 {
+		target = <&test>;
+
+		frag0: __overlay__ {
+			test-int-property = <43>;
+		};
+	};
+
+    test-ref {
+        ref = <&frag0>;
+    };
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 294585b..a65b166 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -329,6 +329,11 @@ dtc_overlay_tests () {
     run_test check_path overlay_base_with_aliases.dtb not-exists "/__symbols__"
     run_test check_path overlay_base_with_aliases.dtb not-exists "/__fixups__"
     run_test check_path overlay_base_with_aliases.dtb not-exists "/__local_fixups__"
+
+    # Test taking a reference to an overlay fragment
+    run_dtc_test -@ -I dts -O dtb -o overlay_base_ref.test.dtb "$SRCDIR/overlay_base_ref.dts"
+    run_dtc_test -@ -I dts -O dtb -o overlay_overlay_ref.test.dtb "$SRCDIR/overlay_overlay_ref.dts"
+    run_wrap_test $FDTOVERLAY -i overlay_base_ref.test.dtb overlay_overlay_ref.test.dtb -o overlay_overlay_ref.fdtoverlay.dtb
 }
 
 tree1_tests () {
-- 
2.26.2

Re: [PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[Paul Barker]

On Sat, 19 Dec 2020 at 17:28, Paul Barker <pbarker-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>
> When applying an overlay fragment, we should take care not to overwrite
> an existing phandle property of the target node as this could break
> references to the target node elsewhere in the base dtb.
>
> In addition to potentially breaking references within the resulting fdt,
> if the overlay is built with symbols enabled (`-@` option to dtc) then
> fdtoverlay will be unable to merge the overlay with a base dtb file.
>
> A new test case is added to check how fdtoverlay handles this case.
> Attempting to apply this test overlay without the fix in this patch
> results in the following output:
>
>     input  = tests/overlay_base_ref.test.dtb
>     output = tests/overlay_overlay_ref.fdtoverlay.dtb
>     overlay[0] = tests/overlay_overlay_ref.test.dtb
>
>     Failed to apply 'tests/overlay_overlay_ref.test.dtb': FDT_ERR_NOTFOUND
>
> In this test case the __overlay__ node in question does not explicitly
> contain a phandle property in the dts file, the phandle is added during
> compilation as it is referenced by another node within the overlay dts.
>
> This failure occurs due to a sequence of events in the functions called
> by fdt_overlay_apply():
>
> 1) In overlay_fixup_phandles(), the target of the overlay fragment is
>    looked up and the target property is set to the phandle of the target
>    node.
>
> 2) In overlay_merge(), the target node is looked up by phandle via
>    overlay_get_target(). As the __overlay__ node in this test case
>    itself has a phandle property, the phandle of the target node is
>    modified.
>
> 3) In overlay_symbol_update(), the target node is again looked up by
>    phandle via overlay_get_target(). But this time the target node
>    cannot be found as its phandle property was modified.
>
> The fix for this issue is to skip modification of the phandle property
> of the target node in step (2) of the above sequence. If the target node
> doesn't already contain a phandle property, we can add one without risk.
>
> Signed-off-by: Paul Barker <pbarker-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  libfdt/fdt_overlay.c          |  2 ++
>  tests/overlay_base_ref.dts    | 19 +++++++++++++++++++
>  tests/overlay_overlay_ref.dts | 24 ++++++++++++++++++++++++
>  tests/run_tests.sh            |  5 +++++
>  4 files changed, 50 insertions(+)
>  create mode 100644 tests/overlay_base_ref.dts
>  create mode 100644 tests/overlay_overlay_ref.dts
>
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index d217e79..b3c217a 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -573,6 +573,8 @@ static int overlay_apply_node(void *fdt, int target,
>                 if (prop_len < 0)
>                         return prop_len;
>
> +               if (!strcmp(name, "phandle") && fdt_getprop(fdt, target, name, NULL))
> +                       continue;
>                 ret = fdt_setprop(fdt, target, name, prop, prop_len);
>                 if (ret)
>                         return ret;
> diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> new file mode 100644
> index 0000000..1fc02a2
> --- /dev/null
> +++ b/tests/overlay_base_ref.dts
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +       test: test-node {
> +               test-int-property = <42>;
> +       };
> +
> +       test-refs {
> +               refs = <&test>;
> +       };
> +};
> diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> new file mode 100644
> index 0000000..a45c95d
> --- /dev/null
> +++ b/tests/overlay_overlay_ref.dts
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +       fragment@0 {
> +               target = <&test>;
> +
> +               frag0: __overlay__ {
> +                       test-int-property = <43>;
> +               };
> +       };
> +
> +    test-ref {
> +        ref = <&frag0>;
> +    };
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 294585b..a65b166 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -329,6 +329,11 @@ dtc_overlay_tests () {
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__symbols__"
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__fixups__"
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__local_fixups__"
> +
> +    # Test taking a reference to an overlay fragment
> +    run_dtc_test -@ -I dts -O dtb -o overlay_base_ref.test.dtb "$SRCDIR/overlay_base_ref.dts"
> +    run_dtc_test -@ -I dts -O dtb -o overlay_overlay_ref.test.dtb "$SRCDIR/overlay_overlay_ref.dts"
> +    run_wrap_test $FDTOVERLAY -i overlay_base_ref.test.dtb overlay_overlay_ref.test.dtb -o overlay_overlay_ref.fdtoverlay.dtb
>  }
>
>  tree1_tests () {
> --
> 2.26.2
>

Hi folks,

I'm sending a ping on this as I haven't heard any feedback in 2 weeks.
Is there anything I can do to help move this forward? Or is there an
alternative approach I should take to solving this issue?

Thanks,

-- 
Paul Barker
Konsulko Group

Re: [PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[David Gibson]

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

On Sun, Jan 03, 2021 at 11:50:19AM +0000, Paul Barker wrote:
> On Sat, 19 Dec 2020 at 17:28, Paul Barker <pbarker-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >
> > When applying an overlay fragment, we should take care not to overwrite
> > an existing phandle property of the target node as this could break
> > references to the target node elsewhere in the base dtb.
> >
> > In addition to potentially breaking references within the resulting fdt,
> > if the overlay is built with symbols enabled (`-@` option to dtc) then
> > fdtoverlay will be unable to merge the overlay with a base dtb file.
> >
> > A new test case is added to check how fdtoverlay handles this case.
> > Attempting to apply this test overlay without the fix in this patch
> > results in the following output:
> >
> >     input  = tests/overlay_base_ref.test.dtb
> >     output = tests/overlay_overlay_ref.fdtoverlay.dtb
> >     overlay[0] = tests/overlay_overlay_ref.test.dtb
> >
> >     Failed to apply 'tests/overlay_overlay_ref.test.dtb': FDT_ERR_NOTFOUND
> >
> > In this test case the __overlay__ node in question does not explicitly
> > contain a phandle property in the dts file, the phandle is added during
> > compilation as it is referenced by another node within the overlay dts.
> >
> > This failure occurs due to a sequence of events in the functions called
> > by fdt_overlay_apply():
> >
> > 1) In overlay_fixup_phandles(), the target of the overlay fragment is
> >    looked up and the target property is set to the phandle of the target
> >    node.
> >
> > 2) In overlay_merge(), the target node is looked up by phandle via
> >    overlay_get_target(). As the __overlay__ node in this test case
> >    itself has a phandle property, the phandle of the target node is
> >    modified.
> >
> > 3) In overlay_symbol_update(), the target node is again looked up by
> >    phandle via overlay_get_target(). But this time the target node
> >    cannot be found as its phandle property was modified.
> >
> > The fix for this issue is to skip modification of the phandle property
> > of the target node in step (2) of the above sequence. If the target node
> > doesn't already contain a phandle property, we can add one without risk.
> >
> > Signed-off-by: Paul Barker <pbarker-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > ---
> >  libfdt/fdt_overlay.c          |  2 ++
> >  tests/overlay_base_ref.dts    | 19 +++++++++++++++++++
> >  tests/overlay_overlay_ref.dts | 24 ++++++++++++++++++++++++
> >  tests/run_tests.sh            |  5 +++++
> >  4 files changed, 50 insertions(+)
> >  create mode 100644 tests/overlay_base_ref.dts
> >  create mode 100644 tests/overlay_overlay_ref.dts
> >
> > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > index d217e79..b3c217a 100644
> > --- a/libfdt/fdt_overlay.c
> > +++ b/libfdt/fdt_overlay.c
> > @@ -573,6 +573,8 @@ static int overlay_apply_node(void *fdt, int target,
> >                 if (prop_len < 0)
> >                         return prop_len;
> >
> > +               if (!strcmp(name, "phandle") && fdt_getprop(fdt, target, name, NULL))
> > +                       continue;
> >                 ret = fdt_setprop(fdt, target, name, prop, prop_len);
> >                 if (ret)
> >                         return ret;
> > diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> > new file mode 100644
> > index 0000000..1fc02a2
> > --- /dev/null
> > +++ b/tests/overlay_base_ref.dts
> > @@ -0,0 +1,19 @@
> > +/*
> > + * Copyright (c) 2016 NextThing Co
> > + * Copyright (c) 2016 Free Electrons
> > + * Copyright (c) 2016 Konsulko Inc.
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +
> > +/dts-v1/;
> > +
> > +/ {
> > +       test: test-node {
> > +               test-int-property = <42>;
> > +       };
> > +
> > +       test-refs {
> > +               refs = <&test>;
> > +       };
> > +};
> > diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> > new file mode 100644
> > index 0000000..a45c95d
> > --- /dev/null
> > +++ b/tests/overlay_overlay_ref.dts
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Copyright (c) 2016 NextThing Co
> > + * Copyright (c) 2016 Free Electrons
> > + * Copyright (c) 2016 Konsulko Inc.
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +
> > +/dts-v1/;
> > +/plugin/;
> > +
> > +/ {
> > +       fragment@0 {
> > +               target = <&test>;
> > +
> > +               frag0: __overlay__ {
> > +                       test-int-property = <43>;
> > +               };
> > +       };
> > +
> > +    test-ref {
> > +        ref = <&frag0>;
> > +    };
> > +};
> > diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> > index 294585b..a65b166 100755
> > --- a/tests/run_tests.sh
> > +++ b/tests/run_tests.sh
> > @@ -329,6 +329,11 @@ dtc_overlay_tests () {
> >      run_test check_path overlay_base_with_aliases.dtb not-exists "/__symbols__"
> >      run_test check_path overlay_base_with_aliases.dtb not-exists "/__fixups__"
> >      run_test check_path overlay_base_with_aliases.dtb not-exists "/__local_fixups__"
> > +
> > +    # Test taking a reference to an overlay fragment
> > +    run_dtc_test -@ -I dts -O dtb -o overlay_base_ref.test.dtb "$SRCDIR/overlay_base_ref.dts"
> > +    run_dtc_test -@ -I dts -O dtb -o overlay_overlay_ref.test.dtb "$SRCDIR/overlay_overlay_ref.dts"
> > +    run_wrap_test $FDTOVERLAY -i overlay_base_ref.test.dtb overlay_overlay_ref.test.dtb -o overlay_overlay_ref.fdtoverlay.dtb
> >  }
> >
> >  tree1_tests () {
> >
> 
> Hi folks,
> 
> I'm sending a ping on this as I haven't heard any feedback in 2 weeks.
> Is there anything I can do to help move this forward? Or is there an
> alternative approach I should take to solving this issue?

Sorry, I've been having trouble finding the time to get my head around
this.  The problem seems real, but I'm not sure if this is the right
approach to fixing it yet.

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

Re: [PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[David Gibson]

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

On Sat, Dec 19, 2020 at 05:28:08PM +0000, Paul Barker wrote:
> When applying an overlay fragment, we should take care not to overwrite
> an existing phandle property of the target node as this could break
> references to the target node elsewhere in the base dtb.

It could.. but my first inclination is to say "don't do that".

> In addition to potentially breaking references within the resulting fdt,
> if the overlay is built with symbols enabled (`-@` option to dtc) then
> fdtoverlay will be unable to merge the overlay with a base dtb file.

If we can manage, I really think the better fix is to *not* have the
overlay provide conflicting phandles, rather than having the merge
process ignore them.

I think we need to pin down the cirucmstances in which overlays with
phandles are being generated and address those, if possible.

> A new test case is added to check how fdtoverlay handles this case.
> Attempting to apply this test overlay without the fix in this patch
> results in the following output:
> 
>     input  = tests/overlay_base_ref.test.dtb
>     output = tests/overlay_overlay_ref.fdtoverlay.dtb
>     overlay[0] = tests/overlay_overlay_ref.test.dtb
> 
>     Failed to apply 'tests/overlay_overlay_ref.test.dtb':
>     FDT_ERR_NOTFOUND

[snip]
> diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> new file mode 100644
> index 0000000..1fc02a2
> --- /dev/null
> +++ b/tests/overlay_base_ref.dts
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +	test: test-node {
> +		test-int-property = <42>;
> +	};
> +
> +	test-refs {
> +		refs = <&test>;
> +	};
> +};
> diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> new file mode 100644
> index 0000000..a45c95d
> --- /dev/null
> +++ b/tests/overlay_overlay_ref.dts
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;

Given that you're using the /plugin/ tag, it doesn't make sense to use
the "manual" way of constructing the overlay, rather than dtc's
built-in syntax:

&test {
	...
}

> +
> +/ {
> +	fragment@0 {
> +		target = <&test>;
> +
> +		frag0: __overlay__ {
> +			test-int-property = <43>;
> +		};
> +	};
> +
> +    test-ref {
> +        ref = <&frag0>;
> +    };
> +};

Things get weird in this example, because the point is we're equating
the __overlay__ node with the target node.  Just ignoring the phandle
overwrite is *wrong* in this case, because it will break test-ref
(except that test-ref isn't in a fragment, and therefore discarded,
but that could be changed).  Instead to handle this case correctly
we need to identify that we're asking the __overlay__ node to be the
same thing as &test and so &frag0 needs to be rewritten as &test.

> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 294585b..a65b166 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -329,6 +329,11 @@ dtc_overlay_tests () {
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__symbols__"
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__fixups__"
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__local_fixups__"
> +
> +    # Test taking a reference to an overlay fragment
> +    run_dtc_test -@ -I dts -O dtb -o overlay_base_ref.test.dtb "$SRCDIR/overlay_base_ref.dts"
> +    run_dtc_test -@ -I dts -O dtb -o overlay_overlay_ref.test.dtb "$SRCDIR/overlay_overlay_ref.dts"
> +    run_wrap_test $FDTOVERLAY -i overlay_base_ref.test.dtb overlay_overlay_ref.test.dtb -o overlay_overlay_ref.fdtoverlay.dtb
>  }
>  
>  tree1_tests () {

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

Re: [PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[Paul Barker]

On Wed, 17 Feb 2021 at 05:25, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Sat, Dec 19, 2020 at 05:28:08PM +0000, Paul Barker wrote:
> > When applying an overlay fragment, we should take care not to overwrite
> > an existing phandle property of the target node as this could break
> > references to the target node elsewhere in the base dtb.
>
> It could.. but my first inclination is to say "don't do that".

I entirely agree. However, folks are already doing this [1] and I
think fdtoverlay should be more defensive here. My idea was to handle
this safely in fdtoverlay if we can assume that it is never legitimate
to modify the phandle property of a node from an overlay. A couple of
alternatives would be raising an error in fdtoverlay instead of trying
to silently ignore the phandle, or raising an error in dtc when the
phandle is introduced.

>
> > In addition to potentially breaking references within the resulting fdt,
> > if the overlay is built with symbols enabled (`-@` option to dtc) then
> > fdtoverlay will be unable to merge the overlay with a base dtb file.
>
> If we can manage, I really think the better fix is to *not* have the
> overlay provide conflicting phandles, rather than having the merge
> process ignore them.
>
> I think we need to pin down the cirucmstances in which overlays with
> phandles are being generated and address those, if possible.

Am I right in understanding this to mean we should raise an error in
dtc when a phandle is generated in an __overlay__ node?

>
> > A new test case is added to check how fdtoverlay handles this case.
> > Attempting to apply this test overlay without the fix in this patch
> > results in the following output:
> >
> >     input  = tests/overlay_base_ref.test.dtb
> >     output = tests/overlay_overlay_ref.fdtoverlay.dtb
> >     overlay[0] = tests/overlay_overlay_ref.test.dtb
> >
> >     Failed to apply 'tests/overlay_overlay_ref.test.dtb':
> >     FDT_ERR_NOTFOUND
>
> [snip]
> > diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> > new file mode 100644
> > index 0000000..1fc02a2
> > --- /dev/null
> > +++ b/tests/overlay_base_ref.dts
> > @@ -0,0 +1,19 @@
> > +/*
> > + * Copyright (c) 2016 NextThing Co
> > + * Copyright (c) 2016 Free Electrons
> > + * Copyright (c) 2016 Konsulko Inc.
> > + *
> > + * SPDX-License-Identifier:  GPL-2.0+
> > + */
> > +
> > +/dts-v1/;
> > +
> > +/ {
> > +     test: test-node {
> > +             test-int-property = <42>;
> > +     };
> > +
> > +     test-refs {
> > +             refs = <&test>;
> > +     };
> > +};
> > diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> > new file mode 100644
> > index 0000000..a45c95d
> > --- /dev/null
> > +++ b/tests/overlay_overlay_ref.dts
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Copyright (c) 2016 NextThing Co
> > + * Copyright (c) 2016 Free Electrons
> > + * Copyright (c) 2016 Konsulko Inc.
> > + *
> > + * SPDX-License-Identifier:  GPL-2.0+
> > + */
> > +
> > +/dts-v1/;
> > +/plugin/;
>
> Given that you're using the /plugin/ tag, it doesn't make sense to use
> the "manual" way of constructing the overlay, rather than dtc's
> built-in syntax:
>
> &test {
>         ...
> }
>
> > +
> > +/ {
> > +     fragment@0 {
> > +             target = <&test>;
> > +
> > +             frag0: __overlay__ {
> > +                     test-int-property = <43>;
> > +             };
> > +     };
> > +
> > +    test-ref {
> > +        ref = <&frag0>;
> > +    };
> > +};
>
> Things get weird in this example, because the point is we're equating
> the __overlay__ node with the target node.  Just ignoring the phandle
> overwrite is *wrong* in this case, because it will break test-ref
> (except that test-ref isn't in a fragment, and therefore discarded,
> but that could be changed).  Instead to handle this case correctly
> we need to identify that we're asking the __overlay__ node to be the
> same thing as &test and so &frag0 needs to be rewritten as &test.

This isn't intended to be an example. It's a minimal test case to
reproduce the bug.

The actual example where this issue was first seen is [1]. The entries
in __overrides__ are interpreted as different options which can be
selected at runtime by a config option. I've looked for the code which
actually implements this but I can't find it, my understanding is that
the proprietary first stage bootloader on the Raspberry Pi applies
these overrides and the overlays together.

In Automotive Grade Linux we want to be able to network boot a
Raspberry Pi and load a device tree with appropriate overlays applied.
The files are fetched over the network by u-boot after the first stage
bootloader has exited so we can't rely on the proprietary Raspberry Pi
firmware to handle this for us. Instead the overlay needs to be
applied either manually at compile time or in u-boot at runtime. This
resulted in the FDT_ERR_NOTFOUND error described in the commit message
for this patch. Discussion and investigation is captured in [2].

[1]: https://github.com/raspberrypi/linux/blob/rpi-5.4.y/arch/arm/boot/dts/overlays/cma-overlay.dts
[2]: https://jira.automotivelinux.org/browse/SPEC-3702

Thanks,

Re: [PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[David Gibson]

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

On Fri, Feb 19, 2021 at 09:28:04AM +0000, Paul Barker wrote:
> On Wed, 17 Feb 2021 at 05:25, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Sat, Dec 19, 2020 at 05:28:08PM +0000, Paul Barker wrote:
> > > When applying an overlay fragment, we should take care not to overwrite
> > > an existing phandle property of the target node as this could break
> > > references to the target node elsewhere in the base dtb.
> >
> > It could.. but my first inclination is to say "don't do that".
> 
> I entirely agree. However, folks are already doing this [1] and I
> think fdtoverlay should be more defensive here. My idea was to handle
> this safely in fdtoverlay if we can assume that it is never legitimate
> to modify the phandle property of a node from an overlay.

The trouble is that both applying the change and ignoring the change
will break something.  Applying it will break things that attempt to
bind to the node with its "old" phandle, but ignoring it will break
something that attempts to bind to the new handle.  e.g.

fragment@1 {
	target = < &somenode >;
	anothername: __overlay__ {
		...
	};		
}
fragment@2 {
	target = < &differentnode >;
	__overlay__ {
		some-prop = < &anothername >;
	};
}

> A couple of
> alternatives would be raising an error in fdtoverlay instead of trying
> to silently ignore the phandle, or raising an error in dtc when the
> phandle is introduced.

Right, I think those are better options.

In the short term, I think raising an error in dtc is probably the
right choice - we can remove it when/if we implement this properly
("merging" the old and new phandles).

> > > In addition to potentially breaking references within the resulting fdt,
> > > if the overlay is built with symbols enabled (`-@` option to dtc) then
> > > fdtoverlay will be unable to merge the overlay with a base dtb file.
> >
> > If we can manage, I really think the better fix is to *not* have the
> > overlay provide conflicting phandles, rather than having the merge
> > process ignore them.
> >
> > I think we need to pin down the cirucmstances in which overlays with
> > phandles are being generated and address those, if possible.
> 
> Am I right in understanding this to mean we should raise an error in
> dtc when a phandle is generated in an __overlay__ node?

As long as we're unable to generate stuff to have that phandle
automatically resolved to the same value as the old phandle, then yes.

> > > A new test case is added to check how fdtoverlay handles this case.
> > > Attempting to apply this test overlay without the fix in this patch
> > > results in the following output:
> > >
> > >     input  = tests/overlay_base_ref.test.dtb
> > >     output = tests/overlay_overlay_ref.fdtoverlay.dtb
> > >     overlay[0] = tests/overlay_overlay_ref.test.dtb
> > >
> > >     Failed to apply 'tests/overlay_overlay_ref.test.dtb':
> > >     FDT_ERR_NOTFOUND
> >
> > [snip]
> > > diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> > > new file mode 100644
> > > index 0000000..1fc02a2
> > > --- /dev/null
> > > +++ b/tests/overlay_base_ref.dts
> > > @@ -0,0 +1,19 @@
> > > +/*
> > > + * Copyright (c) 2016 NextThing Co
> > > + * Copyright (c) 2016 Free Electrons
> > > + * Copyright (c) 2016 Konsulko Inc.
> > > + *
> > > + * SPDX-License-Identifier:  GPL-2.0+
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +/ {
> > > +     test: test-node {
> > > +             test-int-property = <42>;
> > > +     };
> > > +
> > > +     test-refs {
> > > +             refs = <&test>;
> > > +     };
> > > +};
> > > diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> > > new file mode 100644
> > > index 0000000..a45c95d
> > > --- /dev/null
> > > +++ b/tests/overlay_overlay_ref.dts
> > > @@ -0,0 +1,24 @@
> > > +/*
> > > + * Copyright (c) 2016 NextThing Co
> > > + * Copyright (c) 2016 Free Electrons
> > > + * Copyright (c) 2016 Konsulko Inc.
> > > + *
> > > + * SPDX-License-Identifier:  GPL-2.0+
> > > + */
> > > +
> > > +/dts-v1/;
> > > +/plugin/;
> >
> > Given that you're using the /plugin/ tag, it doesn't make sense to use
> > the "manual" way of constructing the overlay, rather than dtc's
> > built-in syntax:
> >
> > &test {
> >         ...
> > }
> >
> > > +
> > > +/ {
> > > +     fragment@0 {
> > > +             target = <&test>;
> > > +
> > > +             frag0: __overlay__ {
> > > +                     test-int-property = <43>;
> > > +             };
> > > +     };
> > > +
> > > +    test-ref {
> > > +        ref = <&frag0>;
> > > +    };
> > > +};
> >
> > Things get weird in this example, because the point is we're equating
> > the __overlay__ node with the target node.  Just ignoring the phandle
> > overwrite is *wrong* in this case, because it will break test-ref
> > (except that test-ref isn't in a fragment, and therefore discarded,
> > but that could be changed).  Instead to handle this case correctly
> > we need to identify that we're asking the __overlay__ node to be the
> > same thing as &test and so &frag0 needs to be rewritten as &test.
> 
> This isn't intended to be an example. It's a minimal test case to
> reproduce the bug.
> 
> The actual example where this issue was first seen is [1]. The entries
> in __overrides__ are interpreted as different options which can be
> selected at runtime by a config option. I've looked for the code which
> actually implements this but I can't find it, my understanding is that
> the proprietary first stage bootloader on the Raspberry Pi applies
> these overrides and the overlays together.

Oh... right.  Well, if you do rely on random extra nodes that aren't
described anywhere in the overlay spec...

Ugh, this really looks like yet more bending dts to do things it was
never really meant to handle.

> In Automotive Grade Linux we want to be able to network boot a
> Raspberry Pi and load a device tree with appropriate overlays applied.
> The files are fetched over the network by u-boot after the first stage
> bootloader has exited so we can't rely on the proprietary Raspberry Pi
> firmware to handle this for us. Instead the overlay needs to be
> applied either manually at compile time or in u-boot at runtime. This
> resulted in the FDT_ERR_NOTFOUND error described in the commit message
> for this patch. Discussion and investigation is captured in [2].
> 
> [1]: https://github.com/raspberrypi/linux/blob/rpi-5.4.y/arch/arm/boot/dts/overlays/cma-overlay.dts
> [2]: https://jira.automotivelinux.org/browse/SPEC-3702
> 
> Thanks,
> 

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

Re: [PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[Paul Barker]

On Mon, 22 Feb 2021 at 06:47, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Fri, Feb 19, 2021 at 09:28:04AM +0000, Paul Barker wrote:
> > On Wed, 17 Feb 2021 at 05:25, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Sat, Dec 19, 2020 at 05:28:08PM +0000, Paul Barker wrote:
> > > > When applying an overlay fragment, we should take care not to overwrite
> > > > an existing phandle property of the target node as this could break
> > > > references to the target node elsewhere in the base dtb.
> > >
> > > It could.. but my first inclination is to say "don't do that".
> >
> > I entirely agree. However, folks are already doing this [1] and I
> > think fdtoverlay should be more defensive here. My idea was to handle
> > this safely in fdtoverlay if we can assume that it is never legitimate
> > to modify the phandle property of a node from an overlay.
>
> The trouble is that both applying the change and ignoring the change
> will break something.  Applying it will break things that attempt to
> bind to the node with its "old" phandle, but ignoring it will break
> something that attempts to bind to the new handle.  e.g.
>
> fragment@1 {
>         target = < &somenode >;
>         anothername: __overlay__ {
>                 ...
>         };
> }
> fragment@2 {
>         target = < &differentnode >;
>         __overlay__ {
>                 some-prop = < &anothername >;
>         };
> }
>
> > A couple of
> > alternatives would be raising an error in fdtoverlay instead of trying
> > to silently ignore the phandle, or raising an error in dtc when the
> > phandle is introduced.
>
> Right, I think those are better options.
>
> In the short term, I think raising an error in dtc is probably the
> right choice - we can remove it when/if we implement this properly
> ("merging" the old and new phandles).
>
> > > > In addition to potentially breaking references within the resulting fdt,
> > > > if the overlay is built with symbols enabled (`-@` option to dtc) then
> > > > fdtoverlay will be unable to merge the overlay with a base dtb file.
> > >
> > > If we can manage, I really think the better fix is to *not* have the
> > > overlay provide conflicting phandles, rather than having the merge
> > > process ignore them.
> > >
> > > I think we need to pin down the cirucmstances in which overlays with
> > > phandles are being generated and address those, if possible.
> >
> > Am I right in understanding this to mean we should raise an error in
> > dtc when a phandle is generated in an __overlay__ node?
>
> As long as we're unable to generate stuff to have that phandle
> automatically resolved to the same value as the old phandle, then yes.
>
> > > > A new test case is added to check how fdtoverlay handles this case.
> > > > Attempting to apply this test overlay without the fix in this patch
> > > > results in the following output:
> > > >
> > > >     input  = tests/overlay_base_ref.test.dtb
> > > >     output = tests/overlay_overlay_ref.fdtoverlay.dtb
> > > >     overlay[0] = tests/overlay_overlay_ref.test.dtb
> > > >
> > > >     Failed to apply 'tests/overlay_overlay_ref.test.dtb':
> > > >     FDT_ERR_NOTFOUND
> > >
> > > [snip]
> > > > diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> > > > new file mode 100644
> > > > index 0000000..1fc02a2
> > > > --- /dev/null
> > > > +++ b/tests/overlay_base_ref.dts
> > > > @@ -0,0 +1,19 @@
> > > > +/*
> > > > + * Copyright (c) 2016 NextThing Co
> > > > + * Copyright (c) 2016 Free Electrons
> > > > + * Copyright (c) 2016 Konsulko Inc.
> > > > + *
> > > > + * SPDX-License-Identifier:  GPL-2.0+
> > > > + */
> > > > +
> > > > +/dts-v1/;
> > > > +
> > > > +/ {
> > > > +     test: test-node {
> > > > +             test-int-property = <42>;
> > > > +     };
> > > > +
> > > > +     test-refs {
> > > > +             refs = <&test>;
> > > > +     };
> > > > +};
> > > > diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> > > > new file mode 100644
> > > > index 0000000..a45c95d
> > > > --- /dev/null
> > > > +++ b/tests/overlay_overlay_ref.dts
> > > > @@ -0,0 +1,24 @@
> > > > +/*
> > > > + * Copyright (c) 2016 NextThing Co
> > > > + * Copyright (c) 2016 Free Electrons
> > > > + * Copyright (c) 2016 Konsulko Inc.
> > > > + *
> > > > + * SPDX-License-Identifier:  GPL-2.0+
> > > > + */
> > > > +
> > > > +/dts-v1/;
> > > > +/plugin/;
> > >
> > > Given that you're using the /plugin/ tag, it doesn't make sense to use
> > > the "manual" way of constructing the overlay, rather than dtc's
> > > built-in syntax:
> > >
> > > &test {
> > >         ...
> > > }
> > >
> > > > +
> > > > +/ {
> > > > +     fragment@0 {
> > > > +             target = <&test>;
> > > > +
> > > > +             frag0: __overlay__ {
> > > > +                     test-int-property = <43>;
> > > > +             };
> > > > +     };
> > > > +
> > > > +    test-ref {
> > > > +        ref = <&frag0>;
> > > > +    };
> > > > +};
> > >
> > > Things get weird in this example, because the point is we're equating
> > > the __overlay__ node with the target node.  Just ignoring the phandle
> > > overwrite is *wrong* in this case, because it will break test-ref
> > > (except that test-ref isn't in a fragment, and therefore discarded,
> > > but that could be changed).  Instead to handle this case correctly
> > > we need to identify that we're asking the __overlay__ node to be the
> > > same thing as &test and so &frag0 needs to be rewritten as &test.
> >
> > This isn't intended to be an example. It's a minimal test case to
> > reproduce the bug.
> >
> > The actual example where this issue was first seen is [1]. The entries
> > in __overrides__ are interpreted as different options which can be
> > selected at runtime by a config option. I've looked for the code which
> > actually implements this but I can't find it, my understanding is that
> > the proprietary first stage bootloader on the Raspberry Pi applies
> > these overrides and the overlays together.
>
> Oh... right.  Well, if you do rely on random extra nodes that aren't
> described anywhere in the overlay spec...
>
> Ugh, this really looks like yet more bending dts to do things it was
> never really meant to handle.

Agreed. I'd be happy to see this turned into an error in dtc, that
would prompt folks to stop doing such crazy things.

I'll have a look at the dtc code when I get chance and see if I can
figure out where we can detect this and raise an error.

Thanks,

-- 
Paul Barker
Konsulko Group

Re: [PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[David Gibson]

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

On Mon, Feb 22, 2021 at 08:39:48AM +0000, Paul Barker wrote:
> On Mon, 22 Feb 2021 at 06:47, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Fri, Feb 19, 2021 at 09:28:04AM +0000, Paul Barker wrote:
> > > On Wed, 17 Feb 2021 at 05:25, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6rL0yjEaa7Ru@public.gmane.orgau> wrote:
> > > >
> > > > On Sat, Dec 19, 2020 at 05:28:08PM +0000, Paul Barker wrote:
> > > > > When applying an overlay fragment, we should take care not to overwrite
> > > > > an existing phandle property of the target node as this could break
> > > > > references to the target node elsewhere in the base dtb.
> > > >
> > > > It could.. but my first inclination is to say "don't do that".
> > >
> > > I entirely agree. However, folks are already doing this [1] and I
> > > think fdtoverlay should be more defensive here. My idea was to handle
> > > this safely in fdtoverlay if we can assume that it is never legitimate
> > > to modify the phandle property of a node from an overlay.
> >
> > The trouble is that both applying the change and ignoring the change
> > will break something.  Applying it will break things that attempt to
> > bind to the node with its "old" phandle, but ignoring it will break
> > something that attempts to bind to the new handle.  e.g.
> >
> > fragment@1 {
> >         target = < &somenode >;
> >         anothername: __overlay__ {
> >                 ...
> >         };
> > }
> > fragment@2 {
> >         target = < &differentnode >;
> >         __overlay__ {
> >                 some-prop = < &anothername >;
> >         };
> > }
> >
> > > A couple of
> > > alternatives would be raising an error in fdtoverlay instead of trying
> > > to silently ignore the phandle, or raising an error in dtc when the
> > > phandle is introduced.
> >
> > Right, I think those are better options.
> >
> > In the short term, I think raising an error in dtc is probably the
> > right choice - we can remove it when/if we implement this properly
> > ("merging" the old and new phandles).
> >
> > > > > In addition to potentially breaking references within the resulting fdt,
> > > > > if the overlay is built with symbols enabled (`-@` option to dtc) then
> > > > > fdtoverlay will be unable to merge the overlay with a base dtb file.
> > > >
> > > > If we can manage, I really think the better fix is to *not* have the
> > > > overlay provide conflicting phandles, rather than having the merge
> > > > process ignore them.
> > > >
> > > > I think we need to pin down the cirucmstances in which overlays with
> > > > phandles are being generated and address those, if possible.
> > >
> > > Am I right in understanding this to mean we should raise an error in
> > > dtc when a phandle is generated in an __overlay__ node?
> >
> > As long as we're unable to generate stuff to have that phandle
> > automatically resolved to the same value as the old phandle, then yes.
> >
> > > > > A new test case is added to check how fdtoverlay handles this case.
> > > > > Attempting to apply this test overlay without the fix in this patch
> > > > > results in the following output:
> > > > >
> > > > >     input  = tests/overlay_base_ref.test.dtb
> > > > >     output = tests/overlay_overlay_ref.fdtoverlay.dtb
> > > > >     overlay[0] = tests/overlay_overlay_ref.test.dtb
> > > > >
> > > > >     Failed to apply 'tests/overlay_overlay_ref.test.dtb':
> > > > >     FDT_ERR_NOTFOUND
> > > >
> > > > [snip]
> > > > > diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> > > > > new file mode 100644
> > > > > index 0000000..1fc02a2
> > > > > --- /dev/null
> > > > > +++ b/tests/overlay_base_ref.dts
> > > > > @@ -0,0 +1,19 @@
> > > > > +/*
> > > > > + * Copyright (c) 2016 NextThing Co
> > > > > + * Copyright (c) 2016 Free Electrons
> > > > > + * Copyright (c) 2016 Konsulko Inc.
> > > > > + *
> > > > > + * SPDX-License-Identifier:  GPL-2.0+
> > > > > + */
> > > > > +
> > > > > +/dts-v1/;
> > > > > +
> > > > > +/ {
> > > > > +     test: test-node {
> > > > > +             test-int-property = <42>;
> > > > > +     };
> > > > > +
> > > > > +     test-refs {
> > > > > +             refs = <&test>;
> > > > > +     };
> > > > > +};
> > > > > diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> > > > > new file mode 100644
> > > > > index 0000000..a45c95d
> > > > > --- /dev/null
> > > > > +++ b/tests/overlay_overlay_ref.dts
> > > > > @@ -0,0 +1,24 @@
> > > > > +/*
> > > > > + * Copyright (c) 2016 NextThing Co
> > > > > + * Copyright (c) 2016 Free Electrons
> > > > > + * Copyright (c) 2016 Konsulko Inc.
> > > > > + *
> > > > > + * SPDX-License-Identifier:  GPL-2.0+
> > > > > + */
> > > > > +
> > > > > +/dts-v1/;
> > > > > +/plugin/;
> > > >
> > > > Given that you're using the /plugin/ tag, it doesn't make sense to use
> > > > the "manual" way of constructing the overlay, rather than dtc's
> > > > built-in syntax:
> > > >
> > > > &test {
> > > >         ...
> > > > }
> > > >
> > > > > +
> > > > > +/ {
> > > > > +     fragment@0 {
> > > > > +             target = <&test>;
> > > > > +
> > > > > +             frag0: __overlay__ {
> > > > > +                     test-int-property = <43>;
> > > > > +             };
> > > > > +     };
> > > > > +
> > > > > +    test-ref {
> > > > > +        ref = <&frag0>;
> > > > > +    };
> > > > > +};
> > > >
> > > > Things get weird in this example, because the point is we're equating
> > > > the __overlay__ node with the target node.  Just ignoring the phandle
> > > > overwrite is *wrong* in this case, because it will break test-ref
> > > > (except that test-ref isn't in a fragment, and therefore discarded,
> > > > but that could be changed).  Instead to handle this case correctly
> > > > we need to identify that we're asking the __overlay__ node to be the
> > > > same thing as &test and so &frag0 needs to be rewritten as &test.
> > >
> > > This isn't intended to be an example. It's a minimal test case to
> > > reproduce the bug.
> > >
> > > The actual example where this issue was first seen is [1]. The entries
> > > in __overrides__ are interpreted as different options which can be
> > > selected at runtime by a config option. I've looked for the code which
> > > actually implements this but I can't find it, my understanding is that
> > > the proprietary first stage bootloader on the Raspberry Pi applies
> > > these overrides and the overlays together.
> >
> > Oh... right.  Well, if you do rely on random extra nodes that aren't
> > described anywhere in the overlay spec...
> >
> > Ugh, this really looks like yet more bending dts to do things it was
> > never really meant to handle.
> 
> Agreed. I'd be happy to see this turned into an error in dtc, that
> would prompt folks to stop doing such crazy things.

Looking a little deeper at what's going on here, there are basically
three different semantic levels here.  They're all being encoded into
the flattened tree format which is causing some confusion.

1) There's the actual device tree information itself.  This uses
   phandles as a way of referencing nodes, no problem so far.

2) There's the overlay fragments.  This refers to things in the layer
   (1) data via those phandles, again no problem so far.

3) There's the automotive linux extension stuff that's trying to
   update the overlay fragments before they're assembled into the
   final DT.

Layer (3) is trying to refer to layer (2) pieces using phandles, and
this is where it really breaks down.  phandles refer to layer (1)
constructs, which might get modified or rearranged by layer (2).  The
overlay resoluion is assumed to resolve all those into phandles that
reference layer (1).

In your specific case where a label is being attached to an
__overlay__ node itself, the ambiguity becomes more obvious.  Are you
trying to refer to (a) the final DT node where that fragment will be
eventually placed, or (b) to a piece of the overlay *before* it is
resolved?

dtc (or the overlay applicator) really has no way to resolve that
ambiguity, except to say that phandles are really only good for
referring to final DT nodes, not to locations in an overlay.

> I'll have a look at the dtc code when I get chance and see if I can
> figure out where we can detect this and raise an error.

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