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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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