Re: [XEN][PATCH v9 02/19] common/device_tree.c: unflatten_device_tree() propagate errors

[Julien Grall <julien@xxxxxxx>]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> This will be useful in dynamic node programming when new dt nodes are unflattend
> during runtime. Invalid device tree node related errors should be propagated
> back to the caller.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
>
> ---
> Changes from v7:
>      Free allocated memory in case of errors when calling unflatten_dt_node.
> ---
> ---
>   xen/common/device_tree.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index c91d54c493..cd9a6a5c93 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -2108,6 +2108,9 @@ static int __init __unflatten_device_tree(const void *fdt,
>       /* First pass, scan for size */
>       start = ((unsigned long)fdt) + fdt_off_dt_struct(fdt);
>       size = unflatten_dt_node(fdt, 0, &start, NULL, NULL, 0);
> +    if ( !size )
> +        return -EINVAL;
> +
>       size = (size | 3) + 1;
>   
>       dt_dprintk("  size is %#lx allocating...\n", size);
> @@ -2125,11 +2128,21 @@ static int __init __unflatten_device_tree(const void 
> *fdt,
>       start = ((unsigned long)fdt) + fdt_off_dt_struct(fdt);
>       unflatten_dt_node(fdt, mem, &start, NULL, &allnextp, 0);
>       if ( be32_to_cpup((__be32 *)start) != FDT_END )
> -        printk(XENLOG_WARNING "Weird tag at end of tree: %08x\n",
> +    {
> +        printk(XENLOG_ERR "Weird tag at end of tree: %08x\n",
>                     *((u32 *)start));
> +        xfree((__be64 *)mem);
I know that 'mem' is an 'unsigned long' so you need to cast it to a 
pointer. But '__be64 *' seems a rather odd choice. Why not using 'void *'?
Better would be to convert 'mem' to a 'void *' as we allow pointer 
arithmetic in Xen. But that can be dealt separately.
> +        return -EINVAL;
> +    }
> +
>       if ( be32_to_cpu(((__be32 *)mem)[size / 4]) != 0xdeadbeefU )
> -        printk(XENLOG_WARNING "End of tree marker overwritten: %08x\n",
> +    {
> +        printk(XENLOG_ERR "End of tree marker overwritten: %08x\n",
>                     be32_to_cpu(((__be32 *)mem)[size / 4]));
> +        xfree((__be64 *)mem);
Same remark here.

> +        return -EINVAL;
> +    }
> +
>       *allnextp = NULL;
>   
>       dt_dprintk(" <- unflatten_device_tree()\n");
Cheers,

--
Julien Grall