Re: [PATCH] mm: khugepaged: fix potential page state corruption

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Thu, Mar 19, 2020 at 09:57:47AM -0700, Yang Shi wrote:
> 
> 
> On 3/19/20 3:49 AM, Kirill A. Shutemov wrote:
> > On Wed, Mar 18, 2020 at 10:39:21PM -0700, Yang Shi wrote:
> > > 
> > > On 3/18/20 5:55 PM, Yang Shi wrote:
> > > > 
> > > > On 3/18/20 5:12 PM, Kirill A. Shutemov wrote:
> > > > > On Thu, Mar 19, 2020 at 07:19:42AM +0800, Yang Shi wrote:
> > > > > > When khugepaged collapses anonymous pages, the base pages would
> > > > > > be freed
> > > > > > via pagevec or free_page_and_swap_cache().  But, the anonymous page may
> > > > > > be added back to LRU, then it might result in the below race:
> > > > > > 
> > > > > >      CPU A                CPU B
> > > > > > khugepaged:
> > > > > >     unlock page
> > > > > >     putback_lru_page
> > > > > >       add to lru
> > > > > >                  page reclaim:
> > > > > >                    isolate this page
> > > > > >                    try_to_unmap
> > > > > >     page_remove_rmap <-- corrupt _mapcount
> > > > > > 
> > > > > > It looks nothing would prevent the pages from isolating by reclaimer.
> > > > > Hm. Why should it?
> > > > > 
> > > > > try_to_unmap() doesn't exclude parallel page unmapping. _mapcount is
> > > > > protected by ptl. And this particular _mapcount pin is reachable for
> > > > > reclaim as it's not part of usual page table tree. Basically
> > > > > try_to_unmap() will never succeeds until we give up the _mapcount on
> > > > > khugepaged side.
> > > > I don't quite get. What does "not part of usual page table tree" means?
> > > > 
> > > > How's about try_to_unmap() acquires ptl before khugepaged?
> > The page table we are dealing with was detached from the process' page
> > table tree: see pmdp_collapse_flush(). try_to_unmap() will not see the
> > pte.
> > 
> > try_to_unmap() can only reach the ptl if split ptl is disabled
> > (mm->page_table_lock is used), but it still will not be able to reach pte.
> 
> Aha, got it. Thanks for explaining. I definitely missed this point. Yes,
> pmdp_collapse_flush() would clear the pmd, then others won't see the page
> table.
> 
> However, it looks the vmscan would not stop at try_to_unmap() at all,
> try_to_unmap() would just return true since pmd_present() should return
> false in pvmw. Then it would go all the way down to __remove_mapping(), but
> freezing the page would fail since try_to_unmap() doesn't actually drop the
> refcount from the pte map.

No. try_to_unmap() checks mapcount at the end and only returns true if
it's zero.

> It would not result in any critical problem AFAICT, but suboptimal and it
> may causes some unnecessary I/O due to swap.
> 
> > 
> > > > > I don't see the issue right away.
> > > > > 
> > > > > > The other problem is the page's active or unevictable flag might be
> > > > > > still set when freeing the page via free_page_and_swap_cache().
> > > > > So what?
> > > > The flags may leak to page free path then kernel may complain if
> > > > DEBUG_VM is set.
> > Could you elaborate on what codepath you are talking about?
> 
> __put_page ->
>     __put_single_page ->
>         free_unref_page ->
>             put_unref_page_prepare ->
>                 free_pcp_prepare ->
>                     free_pages_prepare ->
>                         free_pages_check
> 
> This check would just be run when DEBUG_VM is enabled.

I'm not 100% sure, but I belive these flags will ge cleared on adding into
lru:

  release_pte_page()
    putback_lru_page()
      lru_cache_add()
       __lru_cache_add()
         __pagevec_lru_add()
	   __pagevec_lru_add_fn()
	     __pagevec_lru_add_fn()

-- 
 Kirill A. Shutemov