Skip to content

Commit dd5fb14

Browse files
gormanmbwhacks
authored andcommitted
mm: pin address_space before dereferencing it while isolating an LRU page
commit 69d763f 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/[email protected] Fixes: c824493 ("mm: compaction: make isolate_lru_page() filter-aware again") Signed-off-by: Mel Gorman <[email protected]> Acked-by: Minchan Kim <[email protected]> Cc: "Huang, Ying" <[email protected]> Cc: Jan Kara <[email protected]> Signed-off-by: Andrew Morton <[email protected]> Signed-off-by: Linus Torvalds <[email protected]> [bwh: Backported to 3.16: adjust context] Signed-off-by: Ben Hutchings <[email protected]>
1 parent bf42c14 commit dd5fb14

File tree

1 file changed

+12
-2
lines changed

1 file changed

+12
-2
lines changed

mm/vmscan.c

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1206,6 +1206,7 @@ int __isolate_lru_page(struct page *page, isolate_mode_t mode)
12061206

12071207
if (PageDirty(page)) {
12081208
struct address_space *mapping;
1209+
bool migrate_dirty;
12091210

12101211
/* ISOLATE_CLEAN means only clean pages */
12111212
if (mode & ISOLATE_CLEAN)
@@ -1214,10 +1215,19 @@ int __isolate_lru_page(struct page *page, isolate_mode_t mode)
12141215
/*
12151216
* Only pages without mappings or that have a
12161217
* ->migratepage callback are possible to migrate
1217-
* without blocking
1218+
* without blocking. However, we can be racing with
1219+
* truncation so it's necessary to lock the page
1220+
* to stabilise the mapping as truncation holds
1221+
* the page lock until after the page is removed
1222+
* from the page cache.
12181223
*/
1224+
if (!trylock_page(page))
1225+
return ret;
1226+
12191227
mapping = page_mapping(page);
1220-
if (mapping && !mapping->a_ops->migratepage)
1228+
migrate_dirty = mapping && mapping->a_ops->migratepage;
1229+
unlock_page(page);
1230+
if (!migrate_dirty)
12211231
return ret;
12221232
}
12231233
}

0 commit comments

Comments
 (0)