| Message ID | 20241004122220.234817-6-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: dt overlay fixes | expand |
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@amd.com> Good catch! Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > 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 >
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;
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@amd.com> --- Changes in v2: - New patch --- xen/common/dt-overlay.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)