[v2] mm: migrate: don't rely on __PageMovable() of newpage after unlocking it

Commit Message

David Hildenbrand Jan. 29, 2019, 11:32 p.m. UTC

We had a race in the old balloon compaction code before commit b1123ea6d3b3
("mm: balloon: use general non-lru movable page feature") refactored it
that became visible after backporting commit 195a8c43e93d
("virtio-balloon: deflate via a page list") without the refactoring.

The bug existed from commit d6d86c0a7f8d ("mm/balloon_compaction: redesign
ballooned pages management") till commit b1123ea6d3b3 ("mm: balloon: use
general non-lru movable page feature"). commit d6d86c0a7f8d
("mm/balloon_compaction: redesign ballooned pages management") was
backported to 3.12, so the broken kernels are stable kernels [3.12 - 4.7].

There was a subtle race between dropping the page lock of the newpage
in __unmap_and_move() and checking for
__is_movable_balloon_page(newpage).

Just after dropping this page lock, virtio-balloon could go ahead and
deflate the newpage, effectively dequeueing it and clearing PageBalloon,
in turn making __is_movable_balloon_page(newpage) fail.

This resulted in dropping the reference of the newpage via
putback_lru_page(newpage) instead of put_page(newpage), leading to
page->lru getting modified and a !LRU page ending up in the LRU lists.
With commit 195a8c43e93d ("virtio-balloon: deflate via a page list")
backported, one would suddenly get corrupted lists in
release_pages_balloon():
- WARNING: CPU: 13 PID: 6586 at lib/list_debug.c:59 __list_del_entry+0xa1/0xd0
- list_del corruption. prev->next should be ffffe253961090a0, but was dead000000000100

Nowadays this race is no longer possible, but it is hidden behind very
ugly handling of __ClearPageMovable() and __PageMovable().

__ClearPageMovable() will not make __PageMovable() fail, only
PageMovable(). So the new check (__PageMovable(newpage)) will still hold
even after newpage was dequeued by virtio-balloon.

If anybody would ever change that special handling, the BUG would be
introduced again. So instead, make it explicit and use the information
of the original isolated page before migration.

This patch can be backported fairly easy to stable kernels (in contrast
to the refactoring).

Cc: Andrew Morton <[e-mail]>
Cc: Mel Gorman <[e-mail]>
Cc: "Kirill A. Shutemov" <[e-mail]>
Cc: Michal Hocko <[e-mail]>
Cc: Naoya Horiguchi <[e-mail]>
Cc: Jan Kara <[e-mail]>
Cc: Andrea Arcangeli <[e-mail]>
Cc: Dominik Brodowski <[e-mail]>
Cc: Matthew Wilcox <[e-mail]>
Cc: Vratislav Bendel <[e-mail]>
Cc: Rafael Aquini <[e-mail]>
Cc: Konstantin Khlebnikov <[e-mail]>
Cc: Minchan Kim <[e-mail]>
Cc: [e-mail] # 3.12 - 4.7
Fixes: d6d86c0a7f8d ("mm/balloon_compaction: redesign ballooned pages management")
Reported-by: Vratislav Bendel <[e-mail]>
Acked-by: Michal Hocko <[e-mail]>
Acked-by: Rafael Aquini <[e-mail]>
Signed-off-by: David Hildenbrand <[e-mail]>
---
 mm/migrate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Sasha Levin Jan. 31, 2019, 2:04 a.m. UTC | #1

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: d6d86c0a7f8d mm/balloon_compaction: redesign ballooned pages management.

The bot has tested the following trees: v4.20.5, v4.19.18, v4.14.96, v4.9.153, v4.4.172, v3.18.133.

v4.20.5: Build OK!
v4.19.18: Build OK!
v4.14.96: Build OK!
v4.9.153: Build OK!
v4.4.172: Failed to apply! Possible dependencies:
    1031bc589228 ("lib/vsprintf: add %*pg format specifier")
    14e0a214d62d ("tools, perf: make gfp_compact_table up to date")
    1f7866b4aebd ("mm, tracing: make show_gfp_flags() up to date")
    420adbe9fc1a ("mm, tracing: unify mm flags handling in tracepoints and printk")
    53f9263baba6 ("mm: rework mapcount accounting to enable 4k mapping of THPs")
    7cd12b4abfd2 ("mm, page_owner: track and print last migrate reason")
    7d2eba0557c1 ("mm: add tracepoint for scanning pages")
    c6c919eb90e0 ("mm: use put_page() to free page instead of putback_lru_page()")
    d435edca9288 ("mm, page_owner: copy page owner info during migration")
    d8c1bdeb5d6b ("page-flags: trivial cleanup for PageTrans* helpers")
    eca56ff906bd ("mm, shmem: add internal shmem resident memory accounting")
    edf14cdbf9a0 ("mm, printk: introduce new format string for flags")

v3.18.133: Failed to apply! Possible dependencies:
    2847cf95c68f ("mm/debug-pagealloc: cleanup page guard code")
    48c96a368579 ("mm/page_owner: keep track of page owners")
    7cd12b4abfd2 ("mm, page_owner: track and print last migrate reason")
    94f759d62b2c ("mm/page_owner.c: remove unnecessary stack_trace field")
    c6c919eb90e0 ("mm: use put_page() to free page instead of putback_lru_page()")
    d435edca9288 ("mm, page_owner: copy page owner info during migration")
    e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner")
    e30825f1869a ("mm/debug-pagealloc: prepare boottime configurable on/off")
    eefa864b701d ("mm/page_ext: resurrect struct page extending code for debugging")


How should we proceed with this patch?

--
Thanks,
Sasha

Patch

diff --git a/mm/migrate.c b/mm/migrate.c
index 4512afab46ac..402198816d1a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1135,10 +1135,13 @@  static int __unmap_and_move(struct page *page, struct page *newpage,
 	 * If migration is successful, decrease refcount of the newpage
 	 * which will not free the page because new page owner increased
 	 * refcounter. As well, if it is LRU page, add the page to LRU
-	 * list in here.
+	 * list in here. Use the old state of the isolated source page to
+	 * determine if we migrated a LRU page. newpage was already unlocked
+	 * and possibly modified by its owner - don't rely on the page
+	 * state.
 	 */
 	if (rc == MIGRATEPAGE_SUCCESS) {
-		if (unlikely(__PageMovable(newpage)))
+		if (unlikely(!is_lru))
 			put_page(newpage);
 		else
 			putback_lru_page(newpage);