[PATCH] powerpc: Check flat device tree version at boot

[PATCH] powerpc: Check flat device tree version at boot

[Michael Ellerman]

In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt",
the kernel stopped supporting old flat device tree formats. The minimum
supported version is now 0x10.

There was a checking function added, early_init_dt_verify(), but it's
not called on powerpc.

The result is, if you boot with an old flat device tree, the kernel will
fail to parse it correctly, think you have no memory etc. and hilarity
ensues.

We can't really fix it, but we can at least catch the fact that the
device tree is in an unsupported format and panic(). We can't call
BUG(), it's too early.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/prom.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 4e139f8a69ef..bb777b255b1c 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -641,6 +641,10 @@ void __init early_init_devtree(void *params)
 
 	DBG(" -> early_init_devtree(%p)\n", params);
 
+	/* Too early to BUG_ON(), do it by hand */
+	if (!early_init_dt_verify(params))
+		panic("BUG: Failed verifying flat device tree, bad version?");
+
 	/* Setup flat device-tree pointer */
 	initial_boot_params = params;
 
-- 
1.9.1

Re: [PATCH] powerpc: Check flat device tree version at boot

[Rob Herring]

On Thu, Aug 28, 2014 at 3:40 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt",
> the kernel stopped supporting old flat device tree formats. The minimum
> supported version is now 0x10.

Ugg. Is that something which needs to be supported? Supporting it at
this point would mean adding support to libfdt.

Rob

> There was a checking function added, early_init_dt_verify(), but it's
> not called on powerpc.
>
> The result is, if you boot with an old flat device tree, the kernel will
> fail to parse it correctly, think you have no memory etc. and hilarity
> ensues.
>
> We can't really fix it, but we can at least catch the fact that the
> device tree is in an unsupported format and panic(). We can't call
> BUG(), it's too early.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/prom.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 4e139f8a69ef..bb777b255b1c 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -641,6 +641,10 @@ void __init early_init_devtree(void *params)
>
>         DBG(" -> early_init_devtree(%p)\n", params);
>
> +       /* Too early to BUG_ON(), do it by hand */
> +       if (!early_init_dt_verify(params))
> +               panic("BUG: Failed verifying flat device tree, bad version?");
> +
>         /* Setup flat device-tree pointer */
>         initial_boot_params = params;
>
> --
> 1.9.1
>

Re: [PATCH] powerpc: Check flat device tree version at boot

[Michael Ellerman]

On Thu, 2014-08-28 at 09:27 -0500, Rob Herring wrote:
> On Thu, Aug 28, 2014 at 3:40 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt",
> > the kernel stopped supporting old flat device tree formats. The minimum
> > supported version is now 0x10.
> 
> Ugg. Is that something which needs to be supported? Supporting it at
> this point would mean adding support to libfdt.

Well it would have been nice to keep supporting v2, dropping it broke
kexec-tools for example. But it's done now, so we'll just deal with the
fallout.

If you can CC linuxppc-dev in future on patches that fundamentally change the
device tree parsing that would be appreciated :)

cheers

Re: [PATCH] powerpc: Check flat device tree version at boot

[Grant Likely]

On 29 Aug 2014 02:56, "Michael Ellerman" <mpe@ellerman.id.au> wrote:
>
> On Thu, 2014-08-28 at 09:27 -0500, Rob Herring wrote:
> > On Thu, Aug 28, 2014 at 3:40 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > > In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt",
> > > the kernel stopped supporting old flat device tree formats. The minimum
> > > supported version is now 0x10.
> >
> > Ugg. Is that something which needs to be supported? Supporting it at
> > this point would mean adding support to libfdt.
>
> Well it would have been nice to keep supporting v2, dropping it broke
> kexec-tools for example. But it's done now, so we'll just deal with the
> fallout.

Umm, so we broke userspace? That's not okay. At the very least we
should investigate how much work is needed to bring v2 support into
libfdt, or up-rev the flat tree at runtime before we parse it.

We should be able to update it in-line. IIRC, the main difference
between v2 and v0x10 is that only the leaf node name is encoded into
the node instead of the full name. We can loop over the tree and
truncate all the names while filling the unused bytes with FDT NOP
tags. Should be simple. Something like the following:

fixup_old_device_tree(blob)
{
       for (offset = fdt_next_node(blob, -1, &depth);
             offset >= 0 && depth >= 0 && !rc;
             offset = fdt_next_node(blob, offset, &depth)) {
                char *pathp = fdt_get_name(blob, offset, NULL);
                if (*pathp == '/') {
                        char *leaf = kbasename(pathp);
                        int len = strlen(pathp);
                        int newlen = strlen(leaf);
                        strcpy(pathp, leaf); /* copying in place, need
to check make sure this code is safe */
                        node->size = newlen /* fixme - this is just
pseudocode */
                        newlen = FDT_TAGALIGN(newlen);
                        while (newlen < len) {
                                *(pathp + newlen) = FDT_NOP;
                                newlen = FDT_TAGALIGN(newlen+1);
                        }
                }
       }

}

There's probable some more elegance that can be put into the above
block, but you get the idea. That could be run in-place without
copying the tree to another location, and it could possibly be done in
the boot wrapper. Then we'd also be able to get rid of the v2
compatibility code elsewhere in drivers/of/fdt.c because it would
already be taken care of.

g.

Re: [PATCH] powerpc: Check flat device tree version at boot

[Rob Herring]

On Fri, Aug 29, 2014 at 6:13 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On 29 Aug 2014 02:56, "Michael Ellerman" <mpe@ellerman.id.au> wrote:
>>
>> On Thu, 2014-08-28 at 09:27 -0500, Rob Herring wrote:
>> > On Thu, Aug 28, 2014 at 3:40 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> > > In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt",
>> > > the kernel stopped supporting old flat device tree formats. The minimum
>> > > supported version is now 0x10.
>> >
>> > Ugg. Is that something which needs to be supported? Supporting it at
>> > this point would mean adding support to libfdt.
>>
>> Well it would have been nice to keep supporting v2, dropping it broke
>> kexec-tools for example. But it's done now, so we'll just deal with the
>> fallout.
>
> Umm, so we broke userspace? That's not okay. At the very least we
> should investigate how much work is needed to bring v2 support into
> libfdt, or up-rev the flat tree at runtime before we parse it.

Would up-reving work? kexec-tools is broken or booting a new kernel
with kexec is broken?

> We should be able to update it in-line. IIRC, the main difference
> between v2 and v0x10 is that only the leaf node name is encoded into
> the node instead of the full name. We can loop over the tree and
> truncate all the names while filling the unused bytes with FDT NOP
> tags. Should be simple. Something like the following:

There is also the name property in v1-v3. I'm guessing linux ignores
that already?

>
> fixup_old_device_tree(blob)
> {
>        for (offset = fdt_next_node(blob, -1, &depth);
>              offset >= 0 && depth >= 0 && !rc;
>              offset = fdt_next_node(blob, offset, &depth)) {
>                 char *pathp = fdt_get_name(blob, offset, NULL);
>                 if (*pathp == '/') {
>                         char *leaf = kbasename(pathp);
>                         int len = strlen(pathp);
>                         int newlen = strlen(leaf);
>                         strcpy(pathp, leaf); /* copying in place, need
> to check make sure this code is safe */
>                         node->size = newlen /* fixme - this is just
> pseudocode */
>                         newlen = FDT_TAGALIGN(newlen);
>                         while (newlen < len) {
>                                 *(pathp + newlen) = FDT_NOP;
>                                 newlen = FDT_TAGALIGN(newlen+1);
>                         }
>                 }
>        }
>
> }
>
> There's probable some more elegance that can be put into the above
> block, but you get the idea. That could be run in-place without
> copying the tree to another location, and it could possibly be done in
> the boot wrapper. Then we'd also be able to get rid of the v2
> compatibility code elsewhere in drivers/of/fdt.c because it would
> already be taken care of.

That's what got us into this problem. ;)

Rob