Re: [PATCH 2/12] mm: rmap use pte lock not mmap_sem to set PageMlocked

[Hugh Dickins <hughd@google.com>]

On Mon, 19 Oct 2015, Vlastimil Babka wrote:
> On 10/19/2015 06:50 AM, Hugh Dickins wrote:
> > KernelThreadSanitizer (ktsan) has shown that the down_read_trylock()
> > of mmap_sem in try_to_unmap_one() (when going to set PageMlocked on
> > a page found mapped in a VM_LOCKED vma) is ineffective against races
> > with exit_mmap()'s munlock_vma_pages_all(), because mmap_sem is not
> > held when tearing down an mm.
> > 
> > But that's okay, those races are benign; and although we've believed
> 
> But didn't Kirill show that it's not so benign, and can leak memory?
> - http://marc.info/?l=linux-mm&m=144196800325498&w=2

Kirill's race was this:

		CPU0				CPU1
exit_mmap()
  // mmap_sem is *not* taken
  munlock_vma_pages_all()
    munlock_vma_pages_range()
    					try_to_unmap_one()
					  down_read_trylock(&vma->vm_mm->mmap_sem))
					  !!(vma->vm_flags & VM_LOCKED) == true
      vma->vm_flags &= ~VM_LOCKED;
      <munlock the page>
      					  mlock_vma_page(page);
					  // mlocked pages is leaked.

Hmm, I pulled that in to say that it looked benign to me, that he was
missing all the subsequent "<munlock the page>" which would correct the
situation.  But now I look at it again, I agree with you both: lacking
any relevant locking on CPU1 at that point (it has already given up the
pte lock there), the whole of "<munlock the page>" could take place on
CPU0, before CPU1 reaches its mlock_vma_page(page), yes.

Oh, hold on, no: doesn't page lock prevent that one?  CPU1 has the page
lock throughout, so CPU0's <munlock the page> cannot complete before
CPU1's mlock_vma_page(page).  So now I disagree with you again!

> Although as I noted, it probably doesn't leak completely. But a page will
> remain unevictable, until its last user unmaps it, which is again not
> completely benign?
> - http://marc.info/?l=linux-mm&m=144198536831589&w=2

I agree that we'd be wrong to leave a page on the unevictable lru
indefinitely once it's actually evictable.  But I think my change is
only making the above case easier to think about: trylock on mmap_sem
is a confusing distraction from where the proper locking is done,
whether it be page lock or pte lock.

> 
> 
> > for years in that ugly down_read_trylock(), it's unsuitable for the job,
> > and frustrates the good intention of setting PageMlocked when it fails.
> > 
> > It just doesn't matter if here we read vm_flags an instant before or
> > after a racing mlock() or munlock() or exit_mmap() sets or clears
> > VM_LOCKED: the syscalls (or exit) work their way up the address space
> > (taking pt locks after updating vm_flags) to establish the final state.
> > 
> > We do still need to be careful never to mark a page Mlocked (hence
> > unevictable) by any race that will not be corrected shortly after.
> 
> And waiting for the last user to unmap the page is not necessarily shortly
> after :)
> 
> Anyway pte lock looks like it could work, but I'll need to think about it
> some more, because...
> 
> > The page lock protects from many of the races, but not all (a page
> > is not necessarily locked when it's unmapped).  But the pte lock we
> > just dropped is good to cover the rest (and serializes even with
> > munlock_vma_pages_all(),
> 
> Note how munlock_vma_pages_range() via __munlock_pagevec() does
> TestClearPageMlocked() without (or "between") pte or page lock. But the pte
> lock is being taken after clearing VM_LOCKED, so perhaps it's safe against
> try_to_unmap_one...

A mind-trick I found helpful for understanding the barriers here, is
to imagine that the munlocker repeats its "vma->vm_flags &= ~VM_LOCKED"
every time it takes the pte lock: it does not actually do that, it
doesn't need to of course; but that does help show that ~VM_LOCKED
must be visible to anyone getting that pte lock afterwards.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>