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

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

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.

> > > 
> > > 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?

> > > > The putback_lru_page() would not clear those two flags if the pages are
> > > > released via pagevec, it sounds nothing prevents from isolating active
> 
> Sorry, this is a typo. If the page is freed via pagevec, active and
> unevictable flag would get cleared before freeing by page_off_lru().
> 
> But, if the page is freed by free_page_and_swap_cache(), these two flags are
> not cleared. But, it seems this path is hit rare, the pages are freed by
> pagevec for the most cases.
> 
> > > > or unevictable pages.
> > > Again, why should it? vmscan is equipped to deal with this.
> > 
> > I don't mean vmscan, I mean khugepaged may isolate active and
> > unevictable pages since it just simply walks page table.

Why it is wrong? lru_cache_add() only complains if both flags set, it
shouldn't happen.

> > > > However I didn't really run into these problems, just in theory
> > > > by visual
> > > > inspection.
> > > > 
> > > > And, it also seems unnecessary to have the pages add back to LRU
> > > > again since
> > > > they are about to be freed when reaching this point.  So,
> > > > clearing active
> > > > and unevictable flags, unlocking and dropping refcount from isolate
> > > > instead of calling putback_lru_page() as what page cache collapse does.
> > > Hm? But we do call putback_lru_page() on the way out. I do not follow.
> > 
> > It just calls putback_lru_page() at error path, not success path.
> > Putting pages back to lru on error path definitely makes sense. Here it
> > is the success path.

I agree that putting the apage on LRU just before free the page is
suboptimal, but I don't see it as a critical issue.


-- 
 Kirill A. Shutemov