linux-kernel - [PATCH] mm: khugepaged: fix potential page state corruption

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 19 Mar 2020 07:19:42 +0800
From:   Yang Shi <yang.shi@...ux.alibaba.com>
To:     kirill.shutemov@...ux.intel.com, hughd@...gle.com,
        aarcange@...hat.com, akpm@...ux-foundation.org
Cc:     yang.shi@...ux.alibaba.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: [PATCH] mm: khugepaged: fix potential page state corruption

When khugepaged collapses anonymous pages, the base pages would be freed
via pagevec or free_page_and_swap_cache().  But, the anonymous page may
be added back to LRU, then it might result in the below race:

	CPU A				CPU B
khugepaged:
  unlock page
  putback_lru_page
    add to lru
				page reclaim:
				  isolate this page
				  try_to_unmap
  page_remove_rmap <-- corrupt _mapcount

It looks nothing would prevent the pages from isolating by reclaimer.

The other problem is the page's active or unevictable flag might be
still set when freeing the page via free_page_and_swap_cache().  The
putback_lru_page() would not clear those two flags if the pages are
released via pagevec, it sounds nothing prevents from isolating active
or unevictable pages.

However I didn't really run into these problems, just in theory by visual
inspection.

And, it also seems unnecessary to have the pages add back to LRU again since
they are about to be freed when reaching this point.  So, clearing active
and unevictable flags, unlocking and dropping refcount from isolate
instead of calling putback_lru_page() as what page cache collapse does.

Cc: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
Cc: Hugh Dickins <hughd@...gle.com>
Cc: Andrea Arcangeli <aarcange@...hat.com>
Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
---
 mm/khugepaged.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index b679908..f42fa4e 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -673,7 +673,6 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page,
 			src_page = pte_page(pteval);
 			copy_user_highpage(page, src_page, address, vma);
 			VM_BUG_ON_PAGE(page_mapcount(src_page) != 1, src_page);
-			release_pte_page(src_page);
 			/*
 			 * ptl mostly unnecessary, but preempt has to
 			 * be disabled to update the per-cpu stats
@@ -687,6 +686,15 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page,
 			pte_clear(vma->vm_mm, address, _pte);
 			page_remove_rmap(src_page, false);
 			spin_unlock(ptl);
+
+			dec_node_page_state(src_page,
+				NR_ISOLATED_ANON + page_is_file_cache(src_page));
+			ClearPageActive(src_page);
+			ClearPageUnevictable(src_page);
+			unlock_page(src_page);
+			/* Drop refcount from isolate */
+			put_page(src_page);
+
 			free_page_and_swap_cache(src_page);
 		}
 	}
-- 
1.8.3.1