Re: Crash report: Broken NUMA distance map causes crash on arm64 system

[John Garry <john.garry@huawei.com>]

>>
>> static void free_sched_groups(struct sched_group *sg, int free_sgc)
>> {
>> ...
>>     do {
>>         tmp = sg->next;
>>
>>         if (free_sgc && atomic_dec_and_test(&sg->sgc->ref))***
>>             kfree(sg->sgc);
>>
>> ...
>> }
>>
>> *** crash occurs when free_sgc is non-zero and sg->sgc is NULL
>
> Yeah, turns out to be random memory corruption; I've had the crash in a
> number of weird places; also GCC version dependent.
>
> KASAN is awesome and pinpointed the problem though.
>
>> And, as I mentioned earlier, I bisected this problem to 58d5af59d55b.
>
> You mean:
>
>   051f3ca02e46 ("sched/topology: Introduce NUMA identity node sched domain")
>
> right? and yes indeed! The below fixes my reproducer:
>

Yes, that's the one.

>
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 9d74371e4aad..039578429c25 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1337,7 +1348,7 @@ void sched_init_numa(void)
>  	int level = 0;
>  	int i, j, k;
>
> -	sched_domains_numa_distance = kzalloc(sizeof(int) * nr_node_ids, GFP_KERNEL);
> +	sched_domains_numa_distance = kzalloc(sizeof(int) * (nr_node_ids + 1), GFP_KERNEL);

Good find.

>  	if (!sched_domains_numa_distance)
>  		return;
>
>

So what about this:
 >>> I also note that if I apply the patch, below, to reject the invalid 
NUMA
 >>> distance, we're still getting a warning/error:
 >>>
 >>> [    7.144407] CPU: All CPU(s) started at EL2
 >>> [    7.148678] alternatives: patching kernel code
 >>> [    7.153557] ERROR: Node-0 not representative
 >>> [    7.153557]
 >>> [    7.159365]   10 15 20 25
 >>> [    7.162097]   15 10 25 30
 >>> [    7.164832]   20 25 10 15
 >>> [    7.167562]   25 30 15 10
 >>
 >> Yeah, that's an 'obviously' broken topology too.
 >>
 >
 > AFAICT, this conforms to ACPI spec SLIT rules, and the kernel SLIT
 > validation allows this also. So maybe we should shout louder here or
 > even mark the SLIT as invalid if totally broken.
 >

I plan to fix up OF map parsing to reject invalid distance maps.

However is this distance map so broken for the scheduler that it's 
better to reject this "valid" distance map also? If not, it may be nice 
for the user to know about it without having to enable scheduler debugging.

Thanks,
John