diff mbox series

[v3,2/2] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

Message ID d16b2f8749b65e303f531776d303586336ef1729.1695767747.git.sanastasio@raptorengineering.com (mailing list archive)
State New, archived
Headers show
Series Fix Power CI build | expand

Commit Message

Shawn Anastasio Sept. 26, 2023, 10:37 p.m. UTC 
When building for Power with CONFIG_DEBUG unset, a compiler error gets
raised inside page_alloc.c's node_to_scrub function:

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

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.

Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v2: Add comment to explain the bounds check.

 xen/common/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

--
2.30.2

Comments

Henry Wang Sept. 26, 2023, 11:47 p.m. UTC | #1 
Hi,

> On Sep 27, 2023, at 06:37, Shawn Anastasio <sanastasio@raptorengineering.com> wrote:
> 
> When building for Power with CONFIG_DEBUG unset, a compiler error gets
> raised inside page_alloc.c's node_to_scrub function:
> 
> 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] )
> 
> 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.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

I am seeing some discussions going on about this patch, but once this
is patch is ready for merge please feel free to add:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry


> ---
> v2: Add comment to explain the bounds check.
> 
> xen/common/page_alloc.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 35d9a26fa6..9b5df74fdd 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1211,6 +1211,14 @@ static unsigned int node_to_scrub(bool get_node)
>         } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>                   (node != local_node) );
> 
> +        /*
> +         * In practice `node` will always be within MAX_NUMNODES, but GCC can't
> +         * always see that, so an explicit check is necessary to avoid tripping
> +         * its out-of-bounds array access warning (-Warray-bounds).
> +         */
> +        if ( node >= MAX_NUMNODES )
> +            break;
> +
>         if ( node == local_node )
>             break;
> 
> --
> 2.30.2
> 
>
Jan Beulich Sept. 27, 2023, 6:32 a.m. UTC | #2 
On 27.09.2023 00:37, Shawn Anastasio wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1211,6 +1211,14 @@ static unsigned int node_to_scrub(bool get_node)
>          } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>                    (node != local_node) );
> 
> +        /*
> +         * In practice `node` will always be within MAX_NUMNODES, but GCC can't
> +         * always see that, so an explicit check is necessary to avoid tripping
> +         * its out-of-bounds array access warning (-Warray-bounds).
> +         */
> +        if ( node >= MAX_NUMNODES )
> +            break;
> +
>          if ( node == local_node )
>              break;

My comment on v1 wasn't addressed, either verbally or by a code change. Imo
that would move us a tiny step closer to what Andrew was asking for as well.

Jan
Jan Beulich Sept. 27, 2023, 7:03 a.m. UTC | #3 
On 27.09.2023 08:32, Jan Beulich wrote:
> On 27.09.2023 00:37, Shawn Anastasio wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1211,6 +1211,14 @@ static unsigned int node_to_scrub(bool get_node)
>>          } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>>                    (node != local_node) );
>>
>> +        /*
>> +         * In practice `node` will always be within MAX_NUMNODES, but GCC can't
>> +         * always see that, so an explicit check is necessary to avoid tripping
>> +         * its out-of-bounds array access warning (-Warray-bounds).
>> +         */
>> +        if ( node >= MAX_NUMNODES )
>> +            break;
>> +
>>          if ( node == local_node )
>>              break;
> 
> My comment on v1 wasn't addressed, either verbally or by a code change.

I have to apologize, you did respond, and I didn't spot the response earlier
on. I'm not happy about the added code, but at least it has a comment now.
Hence I guess I simply withdraw my objection, so the change can go in.

Jan

> Imo
> that would move us a tiny step closer to what Andrew was asking for as well.
> 
> Jan
>
diff mbox series

Patch

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 35d9a26fa6..9b5df74fdd 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1211,6 +1211,14 @@  static unsigned int node_to_scrub(bool get_node)
         } while ( !cpumask_empty(&node_to_cpumask(node)) &&
                   (node != local_node) );

+        /*
+         * In practice `node` will always be within MAX_NUMNODES, but GCC can't
+         * always see that, so an explicit check is necessary to avoid tripping
+         * its out-of-bounds array access warning (-Warray-bounds).
+         */
+        if ( node >= MAX_NUMNODES )
+            break;
+
         if ( node == local_node )
             break;