lkml.org 
[lkml]   [2019]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] arm64: numa: check the node id before accessing node_to_cpumask_map
On Fri 30-08-19 16:08:14, Yunsheng Lin wrote:
> On 2019/8/30 14:44, Michal Hocko wrote:
> > 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.
>
> It is only detected by enabling KASAN, the system seems to run fine without
> any visible error if KASAN is disabled. Maybe there is why no report has
> been seen?
>
> Also according to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity
> domain is optional, as below:
>
> This optional object is used to describe proximity domain
> associations within a machine. _PXM evaluates to an integer
> that identifies a device as belonging to a Proximity Domain
> defined in the System Resource Affinity Table (SRAT).
>
>
> Do you think it is ok to resend the fix with above clarification and below
> log:

OK, if the specification really allows to provide NUMA_NO_NODE (-1) then
the code really has to be prepared for that. And ideally all arches
should deal with that.

> [ 42.970381] ==================================================================
> [ 42.977595] BUG: KASAN: global-out-of-bounds in __bitmap_weight+0x48/0xb0
> [ 42.984370] Read of size 8 at addr ffff20008cdf8790 by task kworker/0:1/13
> [ 42.991230]
> [ 42.992712] CPU: 0 PID: 13 Comm: kworker/0:1 Tainted: G O 5.2.0-rc4-g8bde06a-dirty #3
> [ 43.001830] Hardware name: Huawei TaiShan 2280 V2/BC82AMDA, BIOS TA BIOS 2280-A CS V2.B050.01 08/08/2019
> [ 43.011298] Workqueue: events work_for_cpu_fn
> [ 43.015643] Call trace:
> [ 43.018078] dump_backtrace+0x0/0x1e8
> [ 43.021727] show_stack+0x14/0x20
> [ 43.025031] dump_stack+0xc4/0xfc
> [ 43.028335] print_address_description+0x178/0x270
> [ 43.033113] __kasan_report+0x164/0x1b8
> [ 43.036936] kasan_report+0xc/0x18
> [ 43.040325] __asan_load8+0x84/0xa8
> [ 43.043801] __bitmap_weight+0x48/0xb0
> [ 43.047552] hclge_init_ae_dev+0x988/0x1e78 [hclge]
> [ 43.052418] hnae3_register_ae_dev+0xcc/0x278 [hnae3]
> [ 43.057467] hns3_probe+0xe0/0x120 [hns3]
> [ 43.061464] local_pci_probe+0x74/0xf0
> [ 43.065200] work_for_cpu_fn+0x2c/0x48
> [ 43.068937] process_one_work+0x3c0/0x878
> [ 43.072934] worker_thread+0x400/0x670
> [ 43.076670] kthread+0x1b0/0x1b8
> [ 43.079885] ret_from_fork+0x10/0x18
> [ 43.083446]
> [ 43.084925] The buggy address belongs to the variable:
> [ 43.090052] numa_distance+0x30/0x40
> [ 43.093613]
> [ 43.095091] Memory state around the buggy address:
> [ 43.099870] ffff20008cdf8680: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 fa fa
> [ 43.107078] ffff20008cdf8700: fa fa fa fa 04 fa fa fa fa fa fa fa 00 fa fa fa
> [ 43.114286] >ffff20008cdf8780: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
> [ 43.121494] ^
> [ 43.125230] ffff20008cdf8800: 01 fa fa fa fa fa fa fa 04 fa fa fa fa fa fa fa
> [ 43.132439] ffff20008cdf8880: fa fa fa fa fa fa fa fa 00 00 fa fa fa fa fa fa
> [ 43.139646] ==================================================================
>
>
> >
> >>>> 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.
>
> Ok.
>
> How about:
>
> Currently there are different implementations of cpumask_of_node() depend
> on the arch, for example:
>
> ia64:
> #define cpumask_of_node(node) ((node) == -1 ? \
> cpu_all_mask : \
> &node_to_cpu_mask[node])
>
>
> alpha:
> static const struct cpumask *cpumask_of_node(int node)
> {
> int cpu;
>
> if (node == NUMA_NO_NODE)
> return cpu_all_mask;
>
> cpumask_clear(&node_to_cpumask_map[node]);
>
> for_each_online_cpu(cpu) {
> if (cpu_to_node(cpu) == node)
> cpumask_set_cpu(cpu, node_to_cpumask_map[node]);
> }
>
> return &node_to_cpumask_map[node];
> }
>
> Even for the same arch, there are two implementations depend on the
> CONFIG_DEBUG_PER_CPU_MAPS configuration.
>
> arm64/x86 without CONFIG_DEBUG_PER_CPU_MAPS:
> static inline const struct cpumask *cpumask_of_node(int node)
> {
> return node_to_cpumask_map[node];
> }
>
> arm64/x86 with CONFIG_DEBUG_PER_CPU_MAPS:
> const struct cpumask *cpumask_of_node(int node)
> {
> if (WARN_ON(node >= nr_node_ids))
> return cpu_none_mask;
>
> if (WARN_ON(node_to_cpumask_map[node] == NULL))
> return cpu_online_mask;
>
> return node_to_cpumask_map[node];
> }
>
> 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.
--
Michal Hocko
SUSE Labs

\
 
 \ /
  Last update: 2019-08-30 10:40    [W:0.054 / U:0.724 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site