linux-kernel - Re: [PATCH] x86/mm: Drop usage of __flush_tlb_all() in kernel_physical_mapping

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:   Tue, 20 Nov 2018 02:59:32 +0000
From:   "Williams, Dan J" <dan.j.williams@...el.com>
To:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "Hansen, Dave" <dave.hansen@...el.com>
CC:     "bigeasy@...utronix.de" <bigeasy@...utronix.de>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "luto@...nel.org" <luto@...nel.org>, "bp@...en8.de" <bp@...en8.de>
Subject: Re: [PATCH] x86/mm: Drop usage of __flush_tlb_all() in
 kernel_physical_mapping_init()

On Mon, 2018-11-19 at 15:43 -0800, Dave Hansen wrote:
> On 11/19/18 3:19 PM, Dan Williams wrote:
> > Andy wondered why a path that can sleep was using __flush_tlb_all()
> > [1]
> > and Dave confirmed the expectation for TLB flush is for modifying /
> > invalidating existing pte entries, but not initial population [2].
> 
> I _think_ this is OK.
> 
> But, could we sprinkle a few WARN_ON_ONCE(p*_present()) calls in
> there
> to help us sleep at night?

Well, I'm having nightmares now because my naive patch to sprinkle some
WARN_ON_ONCE() calls is leading to my VM live locking at boot... no
backtrace. If I revert the patch below and just go with the
__flush_tlb_all() removal it seems fine.

I'm going to set this aside for a bit, but if anyone has any thoughts
in the meantime I'd appreciate it.

---

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index de95db8ac52f..ecdf917def4c 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -432,6 +432,7 @@ phys_pte_init(pte_t *pte_page, unsigned long paddr, unsigned long paddr_end,
 					     E820_TYPE_RAM) &&
 			    !e820__mapped_any(paddr & PAGE_MASK, paddr_next,
 					     E820_TYPE_RESERVED_KERN))
+				WARN_ON_ONCE(pte_present(*pte));
 				set_pte(pte, __pte(0));
 			continue;
 		}
@@ -452,6 +453,7 @@ phys_pte_init(pte_t *pte_page, unsigned long paddr, unsigned long paddr_end,
 			pr_info("   pte=%p addr=%lx pte=%016lx\n", pte, paddr,
 				pfn_pte(paddr >> PAGE_SHIFT, PAGE_KERNEL).pte);
 		pages++;
+		WARN_ON_ONCE(pte_present(*pte));
 		set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
 		paddr_last = (paddr & PAGE_MASK) + PAGE_SIZE;
 	}
@@ -487,6 +489,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned long paddr, unsigned long paddr_end,
 					     E820_TYPE_RAM) &&
 			    !e820__mapped_any(paddr & PMD_MASK, paddr_next,
 					     E820_TYPE_RESERVED_KERN))
+				WARN_ON_ONCE(pmd_present(*pmd));
 				set_pmd(pmd, __pmd(0));
 			continue;
 		}
@@ -524,6 +527,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned long paddr, unsigned long paddr_end,
 		if (page_size_mask & (1<<PG_LEVEL_2M)) {
 			pages++;
 			spin_lock(&init_mm.page_table_lock);
+			WARN_ON_ONCE(pmd_present(*pmd));
 			set_pte((pte_t *)pmd,
 				pfn_pte((paddr & PMD_MASK) >> PAGE_SHIFT,
 					__pgprot(pgprot_val(prot) | _PAGE_PSE)));
@@ -536,6 +540,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned long paddr, unsigned long paddr_end,
 		paddr_last = phys_pte_init(pte, paddr, paddr_end, new_prot);
 
 		spin_lock(&init_mm.page_table_lock);
+		WARN_ON_ONCE(pmd_present(*pmd));
 		pmd_populate_kernel(&init_mm, pmd, pte);
 		spin_unlock(&init_mm.page_table_lock);
 	}
@@ -573,6 +578,7 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
 					     E820_TYPE_RAM) &&
 			    !e820__mapped_any(paddr & PUD_MASK, paddr_next,
 					     E820_TYPE_RESERVED_KERN))
+				WARN_ON_ONCE(pud_present(*pud));
 				set_pud(pud, __pud(0));
 			continue;
 		}
@@ -610,6 +616,7 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
 		if (page_size_mask & (1<<PG_LEVEL_1G)) {
 			pages++;
 			spin_lock(&init_mm.page_table_lock);
+			WARN_ON_ONCE(pud_present(*pud));
 			set_pte((pte_t *)pud,
 				pfn_pte((paddr & PUD_MASK) >> PAGE_SHIFT,
 					PAGE_KERNEL_LARGE));
@@ -623,6 +630,7 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
 					   page_size_mask, prot);
 
 		spin_lock(&init_mm.page_table_lock);
+		WARN_ON_ONCE(pud_present(*pud));
 		pud_populate(&init_mm, pud, pmd);
 		spin_unlock(&init_mm.page_table_lock);
 	}
@@ -657,6 +665,7 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
 					     E820_TYPE_RAM) &&
 			    !e820__mapped_any(paddr & P4D_MASK, paddr_next,
 					     E820_TYPE_RESERVED_KERN))
+				WARN_ON_ONCE(p4d_present(*p4d));
 				set_p4d(p4d, __p4d(0));
 			continue;
 		}
@@ -674,6 +683,7 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
 					   page_size_mask);
 
 		spin_lock(&init_mm.page_table_lock);
+		WARN_ON_ONCE(p4d_present(*p4d));
 		p4d_populate(&init_mm, p4d, pud);
 		spin_unlock(&init_mm.page_table_lock);
 	}