Re: [PATCH] arm64: numa: check the node id before accessing node_to_cpumask_map

[Michal Hocko]

On Fri 30-08-19 14:35:26, Yunsheng Lin wrote:
> On 2019/8/30 13:55, Michal Hocko wrote:
> > On Fri 30-08-19 10:26:31, Yunsheng Lin wrote:
> >> Some buggy bios may not set the device' numa id, and dev_to_node
> >> will return -1, which may cause global-out-of-bounds error
> >> detected by KASAN.
> > 
> > Why should we workaround a buggy bios like that? Is it so widespread and
> > no BIOS update available? Also, why is this arm64 specific?
> 
> For our case, there is BIOS update available. I just thought it might
> be better to protect from this case when BIOS has not implemented the
> device' numa id setting feature or the feature from BIOS has some bug.
> 
> It is not arm64 specific, right now I only have arm64 board. If it is
> ok to protect this from the buggy BIOS, maybe all other arch can be
> changed too.

If we are to really care then this should be consistent among
architectures IMHO. But I am not really sure this is really worth it.
The code is quite old and I do not really remember any reports. 

> >> This patch changes cpumask_of_node to return cpu_none_mask if the
> >> node is not valid, and sync the cpumask_of_node between the
> >> cpumask_of_node function in numa.h and numa.c.
> > 
> > Why?
> 
> When CONFIG_DEBUG_PER_CPU_MAPS is defined, the cpumask_of_node() in
> numa.c is used, if not, the cpumask_of_node() in numa.h is used.
> 
> I am not sure why there is difference between them, and it is there
> when since the below commit:
> 1a2db300348b ("arm64, numa: Add NUMA support for arm64 platforms.")
> 
> I synced them to keep them consistent whether CONFIG_DEBUG_PER_CPU_MAPS
> is defined.

Such a change should be made in a separate patch with a full
clarification/justification. From the above it is still not clear why
this is needed though.

> >> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> >> ---
> >>  arch/arm64/include/asm/numa.h | 6 ++++++
> >>  arch/arm64/mm/numa.c          | 2 +-
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/include/asm/numa.h b/arch/arm64/include/asm/numa.h
> >> index 626ad01..da891ed 100644
> >> --- a/arch/arm64/include/asm/numa.h
> >> +++ b/arch/arm64/include/asm/numa.h
> >> @@ -25,6 +25,12 @@ const struct cpumask *cpumask_of_node(int node);
> >>  /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
> >>  static inline const struct cpumask *cpumask_of_node(int node)
> >>  {
> >> +	if (node >= nr_node_ids || node < 0)
> >> +		return cpu_none_mask;
> >> +
> >> +	if (!node_to_cpumask_map[node])
> >> +		return cpu_online_mask;
> >> +
> >>  	return node_to_cpumask_map[node];
> >>  }
> >>  #endif
> >> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> >> index 4f241cc..3846313 100644
> >> --- a/arch/arm64/mm/numa.c
> >> +++ b/arch/arm64/mm/numa.c
> >> @@ -46,7 +46,7 @@ EXPORT_SYMBOL(node_to_cpumask_map);
> >>   */
> >>  const struct cpumask *cpumask_of_node(int node)
> >>  {
> >> -	if (WARN_ON(node >= nr_node_ids))
> >> +	if (WARN_ON(node >= nr_node_ids || node < 0))
> >>  		return cpu_none_mask;
> >>  
> >>  	if (WARN_ON(node_to_cpumask_map[node] == NULL))
> >> -- 
> >> 2.8.1
> > 

-- 
Michal Hocko
SUSE Labs