Re: [PATCH 3.16 183/410] mm: pin address_space before dereferencing it while isolating an LRU page

From: Ben Hutchings
Date: Sat Jun 16 2018 - 17:15:47 EST


On Sun, 2018-06-10 at 11:06 -0700, Hugh Dickins wrote:
> On Thu, 7 Jun 2018, Ben Hutchings wrote:
>
> > 3.16.57-rc1 review patch. If anyone has any objections, please let me know.
>
> Not an objection as such, but if you're including this one,
> please be sure to add 145e1a71e090575c74969e3daa8136d1e5b99fc8
> "mm: fix the NULL mapping case in __isolate_lru_page()"

Added, thanks.

Ben.

> Thanks,
> Hugh
>
> >
> > ------------------
> >
> > From: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> >
> > commit 69d763fc6d3aee787a3e8c8c35092b4f4960fa5d upstream.
> >
> > Minchan Kim asked the following question -- what locks protects
> > address_space destroying when race happens between inode trauncation and
> > __isolate_lru_page? Jan Kara clarified by describing the race as follows
> >
> > CPU1 CPU2
> >
> > truncate(inode) __isolate_lru_page()
> > ...
> > truncate_inode_page(mapping, page);
> > delete_from_page_cache(page)
> > spin_lock_irqsave(&mapping->tree_lock, flags);
> > __delete_from_page_cache(page, NULL)
> > page_cache_tree_delete(..)
> > ... mapping = page_mapping(page);
> > page->mapping = NULL;
> > ...
> > spin_unlock_irqrestore(&mapping->tree_lock, flags);
> > page_cache_free_page(mapping, page)
> > put_page(page)
> > if (put_page_testzero(page)) -> false
> > - inode now has no pages and can be freed including embedded address_space
> >
> > if (mapping && !mapping->a_ops->migratepage)
> > - we've dereferenced mapping which is potentially already free.
> >
> > The race is theoretically possible but unlikely. Before the
> > delete_from_page_cache, truncate_cleanup_page is called so the page is
> > likely to be !PageDirty or PageWriteback which gets skipped by the only
> > caller that checks the mappping in __isolate_lru_page. Even if the race
> > occurs, a substantial amount of work has to happen during a tiny window
> > with no preemption but it could potentially be done using a virtual
> > machine to artifically slow one CPU or halt it during the critical
> > window.
> >
> > This patch should eliminate the race with truncation by try-locking the
> > page before derefencing mapping and aborting if the lock was not
> > acquired. There was a suggestion from Huang Ying to use RCU as a
> > side-effect to prevent mapping being freed. However, I do not like the
> > solution as it's an unconventional means of preserving a mapping and
> > it's not a context where rcu_read_lock is obviously protecting rcu data.
> >
> > Link: http://lkml.kernel.org/r/20180104102512.2qos3h5vqzeisrek@xxxxxxxxxxxxxxxxxxx
> > Fixes: c82449352854 ("mm: compaction: make isolate_lru_page() filter-aware again")
> > Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> > Acked-by: Minchan Kim <minchan@xxxxxxxxxx>
> > Cc: "Huang, Ying" <ying.huang@xxxxxxxxx>
> > Cc: Jan Kara <jack@xxxxxxx>
> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > [bwh: Backported to 3.16: adjust context]
> > Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> > ---
> > mm/vmscan.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1206,6 +1206,7 @@ int __isolate_lru_page(struct page *page
> >
> > if (PageDirty(page)) {
> > struct address_space *mapping;
> > + bool migrate_dirty;
> >
> > /* ISOLATE_CLEAN means only clean pages */
> > if (mode & ISOLATE_CLEAN)
> > @@ -1214,10 +1215,19 @@ int __isolate_lru_page(struct page *page
> > /*
> > * Only pages without mappings or that have a
> > * ->migratepage callback are possible to migrate
> > - * without blocking
> > + * without blocking. However, we can be racing with
> > + * truncation so it's necessary to lock the page
> > + * to stabilise the mapping as truncation holds
> > + * the page lock until after the page is removed
> > + * from the page cache.
> > */
> > + if (!trylock_page(page))
> > + return ret;
> > +
> > mapping = page_mapping(page);
> > - if (mapping && !mapping->a_ops->migratepage)
> > + migrate_dirty = mapping && mapping->a_ops->migratepage;
> > + unlock_page(page);
> > + if (!migrate_dirty)
> > return ret;
> > }
> > }
> >
> >
--
Ben Hutchings
The generation of random numbers is too important to be left to chance.
- Robert Coveyou

Attachment: signature.asc
Description: This is a digitally signed message part