linux-kernel - Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker.

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 10 Feb 2017 15:15:48 +0300
From:   Andrey Ryabinin <aryabinin@...tuozzo.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Dmitry Vyukov <dvyukov@...gle.com>
CC:     Ingo Molnar <mingo@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
        "x86@...nel.org" <x86@...nel.org>,
        Tobias Regnery <tobias.regnery@...il.com>,
        "Paul E . McKenney" <paulmck@...ux.vnet.ibm.com>,
        Alexander Potapenko <glider@...gle.com>,
        kasan-dev <kasan-dev@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>
Subject: Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker.



On 02/10/2017 02:18 PM, Thomas Gleixner wrote:
> On Fri, 10 Feb 2017, Dmitry Vyukov wrote:
>> This is the right thing to do per se, but I am concerned that now
>> people will just suffers from slow boot (it can take literally
>> minutes) and will not realize the root cause nor that it's fixable
>> (e.g. with rodata=n) and will probably just blame KASAN for slowness.
>>
>> Could we default this rodata check to n under KASAN? Or at least print
>> some explanatory warning message before doing marking rodata (it
>> should be printed right before "hang", so if you stare at it for a
>> minute during each boot you realize that it may be related)? Or
>> something along these lines. FWIW in my builds I just always disable
>> the check.
> 
> That certainly makes sense and we emit such warnings in other places
> already (lockdep, trace_printk ...)
> 

Agreed, but perhaps it would be better to make this code faster for KASAN=y?
The main problem here is that we have many pgd entries containing kasan_zero_pud values
and ptdump walker checks kasan_zero_pud many times.
Instead, we could check it only once and skip further kasan_zero_pud's.

I can't say I like this hack very much, but it wins me almost 20 seconds of boot time.
Any objections?


diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index 8aa6bea..0fbae1d 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/debugfs.h>
+#include <linux/kasan.h>
 #include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/sched.h>
@@ -121,6 +122,30 @@ static struct addr_marker address_markers[] = {
                        seq_printf(m, fmt, ##args);             \
 })
 
+
+#ifdef CONFIG_KASAN
+static bool kasan_pgd_checked(pgd_t pgd, bool checkwx)
+{
+       static bool kasan_zero_pgd_checked = false;
+       pgd_t kasan_zero_pgd = __pgd(__pa(kasan_zero_pud) | _PAGE_TABLE);
+
+       if (!checkwx)
+               return false;
+
+       if (pgd_val(pgd) == pgd_val(kasan_zero_pgd)) {
+               if (kasan_zero_pgd_checked)
+                       return true;
+               kasan_zero_pgd_checked = true;
+       }
+       return false;
+}
+#else
+static inline bool kasan_pgd_checked(pgd_t pgd, bool checkwx)
+{
+       return false;
+}
+#endif
+
 /*
  * Print a readable form of a pgprot_t to the seq_file
  */
@@ -396,7 +421,8 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
 
        for (i = 0; i < PTRS_PER_PGD; i++) {
                st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
-               if (!pgd_none(*start) && !is_hypervisor_range(i)) {
+               if (!pgd_none(*start) && !is_hypervisor_range(i) &&
+                               !kasan_pgd_checked(*start, checkwx)) {
                        if (pgd_large(*start) || !pgd_present(*start)) {
                                prot = pgd_flags(*start);
                                note_page(m, &st, __pgprot(prot), 1);