Re: [PATCH 1/1] mm/migrate: fix list corruption in migration of non-LRU movable pages

[Minchan Kim]

On Mon, Sep 09, 2019 at 10:40:29AM +0200, Michal Hocko wrote:
> On Thu 05-09-19 01:44:12, sunqiuyang wrote:
> > > 
> > > ________________________________________
> > > From: Michal Hocko [mhocko@xxxxxxxxxx]
> > > Sent: Wednesday, September 04, 2019 20:52
> > > To: sunqiuyang
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx
> > > Subject: Re: [PATCH 1/1] mm/migrate: fix list corruption in migration of non-LRU movable pages
> > > 
> > > On Wed 04-09-19 12:19:11, sunqiuyang wrote:
> > > > > Do not top post please
> > > > >
> > > > > On Wed 04-09-19 07:27:25, sunqiuyang wrote:
> > > > > > isolate_migratepages_block() from another thread may try to isolate the page again:
> > > > > >
> > > > > > for (; low_pfn < end_pfn; low_pfn++) {
> > > > > >   /* ... */
> > > > > >   page = pfn_to_page(low_pfn);
> > > > > >  /* ... */
> > > > > >   if (!PageLRU(page)) {
> > > > > >     if (unlikely(__PageMovable(page)) && !PageIsolated(page)) {
> > > > > >         /* ... */
> > > > > >         if (!isolate_movable_page(page, isolate_mode))
> > > > > >           goto isolate_success;
> > > > > >       /*... */
> > > > > > isolate_success:
> > > > > >      list_add(&page->lru, &cc->migratepages);
> > > > > >
> > > > > > And this page will be added to another list.
> > > > > > Or, do you see any reason that the page cannot go through this path?
> > > > >
> > > > > The page shouldn't be __PageMovable after the migration is done. All the
> > > > > state should have been transfered to the new page IIUC.
> > > > >
> > > >
> > > > I don't see where page->mapping is modified after the migration is done.

Look at __ClearPageMovable which modify page->mapping.
Once driver is migrated the page successfully, it should call __ClearPageMovable.
To not consume new a page flag at that time, this flag is stored at page->mapping
since we already have squeezed several flags in there.

> > > >
> > > > Actually, the last comment in move_to_new_page() says,
> > > > "Anonymous and movable page->mapping will be cleard by
> > > > free_pages_prepare so don't reset it here for keeping
> > > > the type to work PageAnon, for example. "
> > > >
> > > > Or did I miss something? Thanks,
> > > 
> > > This talks about mapping rather than flags stored in the mapping.
> > > I can see that in tree migration handlers (z3fold_page_migrate,
> > > vmballoon_migratepage via balloon_page_delete, zs_page_migrate via
> > > reset_page) all reset the movable flag. I am not sure whether that is a
> > > documented requirement or just a coincidence. Maybe it should be
> > > documented. I would like to hear from Minchan.

It is intended. See Documentation/vm/page_migration.rst

   After isolation, VM calls migratepage of driver with isolated page.
   The function of migratepage is to move content of the old page to new page
   and set up fields of struct page newpage. Keep in mind that you should
   indicate to the VM the oldpage is no longer movable via __ClearPageMovable()
   under page_lock if you migrated the oldpage successfully and returns
   MIGRATEPAGE_SUCCESS.

Thanks.