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

[Vikram Garhwal <vikram.garhwal@amd.com>]

Hi Michal,

On 6/2/23 12:14 AM, Michal Orzel wrote:
> Title: s/unflatten_device_tree/__unflatten_device_tree/ or you mean to propagate
> errors from unflatten_dt_node?
>
> On 02/06/2023 02:48, 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@amd.com>
>> Reviewed-by: Henry Wang <Henry.Wang@arm.com>
>> ---
>>   xen/common/device_tree.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index dfdb10e488..117ccccb09 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,19 @@ 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));
>> +        return -EINVAL;
> What about memory that we allocated? Shouldn't it be freed in case of these two errors?
> For now it is called from boot so we do panic but later in this series this could result
> in a memory leak. Am I correct?
Yeah, that's correct. Let me add the memory free handling in v8.

>> +    }
>> +
>>       if ( be32_to_cpu(((__be32 *)mem)[size / 4]) != 0xdeadbeef )
>> -        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]));
>> +        return -EINVAL;
>> +    }
>> +
>>       *allnextp = NULL;
>>   
>>       dt_dprintk(" <- unflatten_device_tree()\n");
> ~Michal