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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 27 Apr 2018 14:31:51 +0000 From: "Kani, Toshi" <toshi.kani@....com> To: "joro@...tes.org" <joro@...tes.org> CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "bp@...e.de" <bp@...e.de>, "tglx@...utronix.de" <tglx@...utronix.de>, "linux-mm@...ck.org" <linux-mm@...ck.org>, "guohanjun@...wei.com" <guohanjun@...wei.com>, "wxf.wang@...ilicon.com" <wxf.wang@...ilicon.com>, "stable@...r.kernel.org" <stable@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>, "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, "willy@...radead.org" <willy@...radead.org>, "hpa@...or.com" <hpa@...or.com>, "catalin.marinas@....com" <catalin.marinas@....com>, "mingo@...hat.com" <mingo@...hat.com>, "will.deacon@....com" <will.deacon@....com>, "Hocko, Michal" <MHocko@...e.com>, "cpandya@...eaurora.org" <cpandya@...eaurora.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org> Subject: Re: [PATCH v2 2/2] x86/mm: implement free pmd/pte page interfaces On Fri, 2018-04-27 at 09:37 +0200, joro@...tes.org wrote: > On Thu, Apr 26, 2018 at 10:30:14PM +0000, Kani, Toshi wrote: > > Thanks for the clarification. After reading through SDM one more time, I > > agree that we need a TLB purge here. Here is my current understanding. > > > > - INVLPG purges both TLB and paging-structure caches. So, PMD cache was > > purged once. > > - However, processor may cache this PMD entry later in speculation > > since it has p-bit set. (This is where my misunderstanding was. > > Speculation is not allowed to access a target address, but it may still > > cache this PMD entry.) > > - A single INVLPG on each processor purges this PMD cache. It does not > > need a range purge (which was already done). > > > > Does it sound right to you? > > The right fix is to first synchronize the changes when the PMD/PUD is > cleared and then flush the TLB system-wide. After that is done you can > free the page. Agreed. This can be done on top of this patch. > But doing all that in the pud/pmd_free_pmd/pte_page() functions is too > expensive, as the TLB flush requires to send IPIs to all cores in the > system, and that every time the function is called. > > So what needs to be done is to fix this from high-level ioremap code to > first unmap all required PTE/PMD pages and collect them in a list. When > that is done you can synchronize the changes with the other page-tables > in the system and do one system-wide TLB flush. When that is complete > you can free the pages on the list that were collected while unmapping. > > Then the new mappings can be established and again synchronized with the > other page-tables in the system. Yes, and this patch was designed to work in such way. Please note that this patch added pud_free_pmd_page() and pmd_free_pte_page() to the ioremap() path when and only when it creates a pud or pmd map. This assures the following preconditions are met without overhead. - All pte entries for a target pud/pmd address range have been cleared. - System-wide TLB purges have been done for a target pud/pmd address range. So, we can add the step 2 on top of this patch. 1. Clear pud/pmd entry. 2. System wide TLB flush <-- TO BE ADDED BY NEW PATCH 3. Free its underlining pmd/pte page. > > As for the BUG_ON issue, are you able to reproduce this issue? If so, > > would you be able to test the fix? > > Yes, I can reproduce the BUG_ON with my PTI patches and a fedora-i386 > VM. Great! > I already ran into the issue before your patches were merged upstream, > but my "fix" is different because it just prevents huge-mappings when > there were smaller mappings before. See > > e3e288121408 x86/pgtable: Don't set huge PUD/PMD on non-leaf entries > > for details. This patch does not fix the base-problem, but hides it > again, as the real fix needs some more work across architectures. Right. Patch 1/2 of this series made the same fix as well. See: b6bdb7517c3d mm/vmalloc: add interfaces to free unmapped page table > Your patch actually makes the problem worse, without it the PTE/PMD pages > were just leaked, so that they could not be reused. But with your patch > the pages can be used again and the page-walker might establish TLB > entries based on random content the new owner writes to it. This can > lead to all kinds of random and very hard to debug data corruption > issues. > > So until we make the generic ioremap code in lib/ioremap.c smarter about > unmapping/remapping ranges the best solution is making my fix work again > by reverting your patch. We do not need to revert this patch. We can make the above change I mentioned. Thanks, -Toshi
Powered by blists - more mailing lists