Re: [patch 2/3] mm: memcontrol: rewrite uncharge API fix - double migration

[Johannes Weiner]

Hi Hugh,

On Mon, Jul 14, 2014 at 12:57:33PM -0700, Hugh Dickins wrote:
> On Mon, 7 Jul 2014, Johannes Weiner wrote:
> 
> > Hugh reports:
> > 
> > VM_BUG_ON_PAGE(!(pc->flags & PCG_MEM))
> > mm/memcontrol.c:6680!
> > page had count 1 mapcount 0 mapping anon index 0x196
> > flags locked uptodate reclaim swapbacked, pcflags 1, memcg not root
> > mem_cgroup_migrate < move_to_new_page < migrate_pages < compact_zone <
> > compact_zone_order < try_to_compact_pages < __alloc_pages_direct_compact <
> > __alloc_pages_nodemask < alloc_pages_vma < do_huge_pmd_anonymous_page <
> > handle_mm_fault < __do_page_fault
> > 
> > mem_cgroup_migrate() assumes that a page is only migrated once and
> > then freed immediately after.
> > 
> > However, putting the page back on the LRU list and dropping the
> > isolation refcount is not done atomically.  This allows a PFN-based
> > migrator like compaction to isolate the page, see the expected
> > anonymous page refcount of 1, and migrate the page once more.
> > 
> > Catch pages that have already been migrated and abort migration
> > gracefully.
> > 
> > Reported-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> > ---
> >  mm/memcontrol.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 1e3b27f8dc2f..e4afdbdda0a7 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -6653,7 +6653,10 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
> >  	if (!PageCgroupUsed(pc))
> >  		return;
> >  
> > -	VM_BUG_ON_PAGE(!(pc->flags & PCG_MEM), oldpage);
> > +	/* Already migrated */
> > +	if (!(pc->flags & PCG_MEM))
> > +		return;
> > +
> 
> I am curious why you chose to fix the BUG in this way, instead of
> -	pc->flags &= ~(PCG_MEM | PCG_MEMSW);
> +	pc->flags = 0;
> a few lines further down.
> 
> The page that gets left behind with just PCG_USED is anomalous (for an
> LRU page, maybe not for a kmem page), isn'it it?  And liable to cause
> other problems.
> 
> For example, won't it go the wrong way in the "Surreptitiously" test
> in mem_cgroup_page_lruvec(): the page no longer has a hold on any
> memcg, so is in a danger of being placed on a gone-memcg's LRU?

I was worried about unusing the page before we have exclusive access
to it (migration_entry_to_page() can still work at this point, though
the current situation seems safe).

But you are right, with the charge belonging to the new page, the old
page no longer pins the memcg and we have to prevent use-after-free.

How about this as a drop-in replacement?

---