Re: [PATCH 3/3] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.09.2023 00:42, Shawn Anastasio wrote:
> When building for Power with CONFIG_DEBUG unset, a compiler error gets
> raised inside page_alloc.c's node_to_scrub function, likely due to the
> increased optimization level:
> 
> common/page_alloc.c: In function 'node_to_scrub.part.0':
> common/page_alloc.c:1217:29: error: array subscript 1 is above array
>             bounds of 'long unsigned int[1]' [-Werror=array-bounds]
>  1217 |         if ( node_need_scrub[node] )

That's gcc13?

> It appears that this is a false positive, given that in practice
> cycle_node should never return a node ID >= MAX_NUMNODES as long as the
> architecture's node_online_map is properly defined and initialized, so
> this additional bounds check is only to satisfy GCC.

Looks very similar to the situation that c890499871cc ("timer: fix
NR_CPUS=1 build with gcc13") was dealing with, just that here it's
MAX_NUMNODES. I'd therefore prefer a solution similar to the one
taken there, i.e. make code conditional rather than add yet more
code.

Otherwise, ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1211,6 +1211,9 @@ static unsigned int node_to_scrub(bool get_node)
>          } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>                    (node != local_node) );
> 
> +        if ( node >= MAX_NUMNODES )
> +            break;

... this clearly redundant check would need to gain a comment.

What I'm puzzled by is that on Arm we had no reports of a similar
problem, despite NUMA also not getting selected there (yet).

Jan