Re: [PATCH] bootfdt: Fix infinite loop in device_tree_for_each_node()

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 20-Jun-26 13:49, Dmytro Prokopchuk1 wrote:
> When a node's depth exceeds DEVICE_TREE_MAX_DEPTH inside the
> device_tree_for_each_node() loop, the code prints a warning and
> executes 'continue;' statement, which jumps to condition check,
> bypassing the iterator update step:
> 
>     node = fdt_next_node(fdt, node, &depth).
> 
> The node and depth are not updated, the loop repeatedly evaluates
> the same too-deep node, causing a hang.
> 
> Fix this by wrapping the node processing logic in an 'else' block.
> This ensures the loop update step is executed on every iteration,
> safely skipping deeply nested nodes and doing the traversal.
> 
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
Please add a fixes tag:
Fixes: 40f2ea3df2e2 ("xen/arm: pass node to device_tree_for_each_node")

> ---
> 
> Test CI pipeline: 
> https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/2615174670
> 
> Local tests.
> Tests were based on "qemu-xtf.sh".
> In the "/chosen" node were added these "levelN" nesting nodes:
> 
>       chosen {
>               stdout-path = "/pl011@9000000";
>               kaslr-seed = <0x6ae81a67 0x26e92d62>;
>         level1 {
>             level2 {
>         ...
>                         level19 {
>                             level20 {
>                                 compatible = "test";
>                                 value = <1234>;
>                             };
>                         };
>         ...
>             };
>         };
>       };
> 
> Without a patch Xen stuck printing the same message in a loop:
> 
> (XEN) Checking for initrd in /chosen
> (XEN) Checking for "xen,static-mem" in domain node
> (XEN) Warning: device tree node `level15' is nested too deep
> (XEN) Warning: device tree node `level15' is nested too deep
> (XEN) Warning: device tree node `level15' is nested too deep
> (XEN) Warning: device tree node `level15' is nested too deep
> (XEN) Warning: device tree node `level15' is nested too deep
> ...
> 
> With a patch these too-deep nodes were successfully skipped and Xen
> continued to boot:
> 
> (XEN) Checking for initrd in /chosen
> (XEN) Checking for "xen,static-mem" in domain node
> (XEN) Warning: device tree node `level15' is nested too deep
> (XEN) Warning: device tree node `level16' is nested too deep
> (XEN) Warning: device tree node `level17' is nested too deep
> (XEN) Warning: device tree node `level18' is nested too deep
> (XEN) Warning: device tree node `level19' is nested too deep
> (XEN) Warning: device tree node `level20' is nested too deep
> (XEN) RAM: 0000000040000000 - 00000000bfffffff
> (XEN) 
> (XEN) MODULE[0]: 0000000043200000 - 000000004337afff Xen         
> (XEN) MODULE[1]: 0000000043400000 - 0000000043402fff Device Tree 
> (XEN) MODULE[2]: 0000000043000000 - 00000000430ef7f6 Ramdisk     
> (XEN) MODULE[3]: 0000000040600000 - 0000000042f4ffff Kernel      
> (XEN) MODULE[4]: 0000000040400000 - 0000000040412fff Kernel      
> (XEN) 
> (XEN) CMDLINE[0000000040600000]:domU0 console=ttyAMA0
> ...
> 
> ---
>  xen/common/device-tree/bootfdt.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/common/device-tree/bootfdt.c 
> b/xen/common/device-tree/bootfdt.c
> index 7c790b9a4d..4d10013b2d 100644
> --- a/xen/common/device-tree/bootfdt.c
> +++ b/xen/common/device-tree/bootfdt.c
> @@ -90,23 +90,24 @@ int __init device_tree_for_each_node(const void *fdt, int 
> node,
>          {
>              printk("Warning: device tree node `%s' is nested too deep\n",
>                     name);
> -            continue;
>          }
> -
> -        as = depth > 0 ? address_cells[depth-1] : 
> DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
> -        ss = depth > 0 ? size_cells[depth-1] : 
> DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
> -
> -        address_cells[depth] = device_tree_get_u32(fdt, node,
> -                                                   "#address-cells", as);
> -        size_cells[depth] = device_tree_get_u32(fdt, node,
> -                                                "#size-cells", ss);
> -
> -        /* skip the first node */
> -        if ( node != first_node )
> +        else
>          {
> -            ret = func(fdt, node, name, depth, as, ss, data);
> -            if ( ret != 0 )
> -                return ret;
> +            as = depth > 0 ? address_cells[depth-1] : 
> DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
> +            ss = depth > 0 ? size_cells[depth-1] : 
> DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
The added indentation level pushes these two lines over 80 columns (the
ss= line was within 80 before this patch). Please wrap them while you
are touching them. With that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>


You don't carry [for-4.22] prefix, but I think it is simple enough to take it
in, so we can ask Oleksii for his opinion to take it for 4.22. @Oleksii?

~Michal

> +
> +            address_cells[depth] = device_tree_get_u32(fdt, node,
> +                                                       "#address-cells", as);
> +            size_cells[depth] = device_tree_get_u32(fdt, node,
> +                                                    "#size-cells", ss);
> +
> +            /* skip the first node */
> +            if ( node != first_node )
> +            {
> +                ret = func(fdt, node, name, depth, as, ss, data);
> +                if ( ret != 0 )
> +                    return ret;
> +            }
>          }
>  
>          node = fdt_next_node(fdt, node, &depth);