Re: [PATCH v2 2/2] x86/mm: implement free pmd/pte page interfaces

[Kani, Toshi]

On Fri, 2018-04-27 at 09:37 +0200, joro@xxxxxxxxxx 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