Re: [PATCH v5 7/9] mm/mremap: Move TLB flush outside page table lock

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 5/21/21 11:43 AM, Linus Torvalds wrote:
> On Thu, May 20, 2021 at 5:03 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> On 5/21/21 8:10 AM, Linus Torvalds wrote:
>>>
>>> So mremap does need to flush the TLB before releasing the page table
>>> lock, because that's the lifetime boundary for the page that got
>>> moved.
>>
>> How will we avoid that happening with
>> c49dd340180260c6239e453263a9a244da9a7c85 /
>> 2c91bd4a4e2e530582d6fd643ea7b86b27907151 . The commit improves mremap
>> performance by moving level3/level2 page table entries. When doing so we
>> are not holding level 4 ptl lock (pte_lock()). But rather we are holding
>> pmd_lock or pud_lock(). So if we move pages around without holding the
>> pte lock, won't the above issue happen even if we do a tlb flush with
>> holding pmd lock/pud lock?
> 
> Hmm. Interesting.
> 
> Your patch (to flush the TLB after clearing the old location, and
> before inserting it into the new one) looks like an "obvious" fix.
> 
> But I'm putting that "obvious" in quotes, because I'm now wondering if
> it actually fixes anything.
> 
> Lookie here:
> 
>   - CPU1 does a mremap of a pmd or pud.
> 
>      It clears the old pmd/pud, flushes the old TLB range, and then
> inserts the pmd/pud at the new location.
> 
>   - CPU2 does a page shrinker, which calls try_to_unmap, which calls
> try_to_unmap_one.
> 
> These are entirely asynchronous, because they have no shared lock. The
> mremap uses the pmd lock, the try_to_unmap_one() does the rmap walk,
> which does the pte lock.
> 
> Now, imagine that the following ordering happens with the two
> operations above, and a CPU3 that does accesses:
> 
>   - CPU2 follows (and sees) the old page tables in the old location and
> the took the pte lock
> 
>   - the mremap on CPU1 starts - cleared the old pmd, flushed the tlb,
> *and* inserts in the new place.
> 
>   - a user thread on CPU3 accesses the new location and fills the TLB
> of the *new* address
> 
>   - only now does CPU2 get to the "pte_get_and_clear()" to remove one page
> 
>   - CPU2 does a TLB flush and frees the page
> 
> End result:
> 
>   - both CPU1 _and_ CPU2 have flushed the TLB.
> 
>   - but both flushed the *OLD* address
> 
>   - the page is freed
> 
>   - CPU3 still has the stale TLB entry pointing to the page that is now
> free and might be reused for something else
> 
> Am I missing something?
> 

That is a problem. With that it looks like CONFIG_HAVE_MOVE_PMD/PUD is 
broken? I don't see an easy way to fix this?

-aneesh