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

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Nov 2018 10:10:48 +0000
From:   John Garry <john.garry@...wei.com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "Anshuman Khandual" <anshuman.khandual@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        <linux-kernel@...r.kernel.org>, Linuxarm <linuxarm@...wei.com>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Ingo Molnar <mingo@...hat.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        <suravee.suthikulpanit@....com>
Subject: Re: Crash report: Broken NUMA distance map causes crash on arm64
 system

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