linux-kernel - Re: [PATCH] arm64: numa: check the node id before accessing node_to_cpumask

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 30 Aug 2019 13:13:43 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Yunsheng Lin <linyunsheng@...wei.com>
Cc:     will@...nel.org, akpm@...ux-foundation.org, rppt@...ux.ibm.com,
        anshuman.khandual@....com, adobriyan@...il.com, cai@....pw,
        robin.murphy@....com, tglx@...utronix.de,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linuxarm@...wei.com
Subject: Re: [PATCH] arm64: numa: check the node id before accessing
 node_to_cpumask_map

On Fri 30-08-19 17:49:46, Yunsheng Lin wrote:
> On 2019/8/30 16:39, Michal Hocko wrote:
> > On Fri 30-08-19 16:08:14, Yunsheng Lin wrote:
[...]
> >> It seems the cpumask_of_node with CONFIG_DEBUG_PER_CPU_MAPS is used
> >> to catch the erorr case and give a warning to user when node id is not
> >> valid.
> > 
> > Yeah the config help text
> > config DEBUG_PER_CPU_MAPS
> >         bool "Debug access to per_cpu maps"
> >         depends on DEBUG_KERNEL
> >         depends on SMP
> >         help
> >           Say Y to verify that the per_cpu map being accessed has
> >           been set up. This adds a fair amount of code to kernel memory
> >           and decreases performance.
> > 
> >           Say N if unsure.
> > 
> > suggests that this is intentionally hidden behind a config so a normal
> > path shouldn't really duplicate it. If those checks make sense in
> > general then the config option should be dropped I think.
> 
> It seems cpumask_of_node with CONFIG_DEBUG_PER_CPU_MAPS on may be used to
> debug some early use of cpumask_of_node problem, see below:
> 
> /*
>  * Allocate node_to_cpumask_map based on number of available nodes
>  * Requires node_possible_map to be valid.
>  *
>  * Note: cpumask_of_node() is not valid until after this is done.
>  * (Use CONFIG_DEBUG_PER_CPU_MAPS to check this.)
>  */
> static void __init setup_node_to_cpumask_map(void)
> {
> 	int node;
> 
> 	/* setup nr_node_ids if not done yet */
> 	if (nr_node_ids == MAX_NUMNODES)
> 		setup_nr_node_ids();
> 
> 	/* allocate and clear the mapping */
> 	for (node = 0; node < nr_node_ids; node++) {
> 		alloc_bootmem_cpumask_var(&node_to_cpumask_map[node]);
> 		cpumask_clear(node_to_cpumask_map[node]);
> 	}
> 
> 	/* cpumask_of_node() will now work */
> 	pr_debug("Node to cpumask map for %u nodes\n", nr_node_ids);
> }
> 
> So I prefer to keep it as two implementations for arm64 and x86, but
> ensure the two implementations be consistent. It can be cleaned up later
> when there is no use at all.
> 
> Is it ok with you?

I am not really sure what you are asking here TBH. You want both
CONFIG_DEBUG_PER_CPU_MAPS implementations to use the same (duplicated) code?
If that is the case then no objections from me. I would obviously
preferred a shared code but that might be a larger change indeed and can
be done later.

-- 
Michal Hocko
SUSE Labs