Re: [PATCH v2 5/6] dt-overlay: Ignore nodes that do not have __overlay__ as their subnode

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 4 Oct 2024, Michal Orzel wrote:
> Assumption stated in the comments as if fdt_for_each_subnode() checks
> for parent < 0 is utterly wrong. If parent is < 0, node offset is set to
> 0 (i.e. the very first node in the tree) and the loop's body is executed.
> This incorrect assumption causes overlay_node_count() to also count nodes
> that do not have __overlay__ as their subnode. The same story goes for
> overlay_get_nodes_info(), where we end up requiring each node directly
> under root node to have "target-path" set. DTBOs can specify other nodes
> including special ones like __symbols__, __fixups__ that can be left to
> reduce the number of steps a user needs to do to when it comes to invalid
> phandles.
> 
> Fix it by adding checks if overlay < 0 after respective calls to
> fdt_subnode_offset().
> 
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>

Good catch!

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>



> ---
> Changes in v2:
>  - New patch
> ---
>  xen/common/dt-overlay.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
> index bfa153250922..4d75b5b36a99 100644
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -274,11 +274,9 @@ static unsigned int overlay_node_count(const void 
> *overlay_fdt)
>          int overlay;
>  
>          overlay = fdt_subnode_offset(overlay_fdt, fragment, "__overlay__");
> +        if ( overlay < 0 )
> +            continue;
>  
> -        /*
> -         * overlay value can be < 0. But fdt_for_each_subnode() loop checks 
> for
> -         * overlay >= 0. So, no need for a overlay>=0 check here.
> -         */
>          fdt_for_each_subnode(subnode, overlay_fdt, overlay)
>          {
>              num_overlay_nodes++;
> @@ -305,6 +303,10 @@ static int overlay_get_nodes_info(const void *fdto, char 
> **nodes_full_path)
>          int subnode;
>          const char *target_path;
>  
> +        overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +        if ( overlay < 0 )
> +            continue;
> +
>          target = fdt_overlay_target_offset(device_tree_flattened, fdto,
>                                             fragment, &target_path);
>          if ( target < 0 )
> @@ -313,12 +315,6 @@ static int overlay_get_nodes_info(const void *fdto, char 
> **nodes_full_path)
>          if ( target_path == NULL )
>              return -EINVAL;
>  
> -        overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> -
> -        /*
> -         * overlay value can be < 0. But fdt_for_each_subnode() loop checks 
> for
> -         * overlay >= 0. So, no need for a overlay>=0 check here.
> -         */
>          fdt_for_each_subnode(subnode, fdto, overlay)
>          {
>              const char *node_name = NULL;
> -- 
> 2.25.1
>