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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 9/26/23 2:32 AM, Jan Beulich wrote:
> 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?
> 

Gcc 10.2.1, actually.

>> 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.
> 

Looking at that commit, it doesn't look like conditionalizing this code
in the same way would be as clean -- it'd likely require separate
conditional blocks around the initial variable declarations and the
function body.

> 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.
>

This isn't a problem. I'll submit a v2 with a commit.

> 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).
>

As you pointed out in your follow-up email, it might be due to
CONFIG_DEBUG. It might also be because on ppc, our implementation of
find_next_bit (which is used by cycle_node) is declared static inline,
so the compiler has access to its definition when doing this sort of
static analysis. On Arm I believe the function lives in its own
compilation unit which might preclude GCC from making any assumptions
about its return value.

> Jan

Thanks,
Shawn