[PATCH] mm/madvise: keep the hardware dirty bit in MADV_COLD's THP path

[samarth.colleges@gmail.com]

From: Samarth Narang <samarth.colleges@gmail.com>

For a young, PMD-mapped THP, madvise_cold_or_pageout_pte_range() handles
MADV_COLD by reading orig_pmd = *pmd, calling pmdp_invalidate(),
discarding its return value, and writing back pmd_mkold(orig_pmd):

	if (!pageout && pmd_young(orig_pmd)) {
		pmdp_invalidate(vma, addr, pmd);
		orig_pmd = pmd_mkold(orig_pmd);
		set_pmd_at(mm, addr, pmd, orig_pmd);
		tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
	}

pmdp_invalidate() atomically clears the present bit and returns the PMD
as it was in the page table at that instant.  A store from another CPU
that lands after orig_pmd is read but before the invalidate sets the
hardware dirty bit in the live entry.  Because the written-back value is
derived from the stale orig_pmd, that dirty bit is erased.

If the THP had been marked lazyfree (MADV_FREE), reclaim later finds a
clean PMD and a clean folio and discards it rather than swapping it out
(__discard_anon_folio_pmd_locked()).  The store is then silently lost: a
subsequent read returns stale/zero data, although the MADV_FREE contract
is that a write after the advice call cancels the free.

pmdp_invalidate() returns the pre-invalidate entry precisely so callers
can carry the accessed/dirty bits hardware may have set concurrently
(d52605d7cb30).  change_huge_pmd() and set_pmd_migration_entry()
(8a8683ad9ba4) already do this; the MADV_COLD path does not.  Rebuild
orig_pmd from the returned entry.

Reproduced unprivileged on x86-64: a 2 MiB anon THP is marked MADV_FREE,
then MADV_COLD races a store from another CPU, then MADV_PAGEOUT; the
stored byte reads back zero.  On current mainline built and booted under
KVM, the race lost 131 of 8000 stores before this patch and 0 of 8000
after; a store-before-COLD control and a MADV_NORMAL control lose none.

Fixes: 9c276cc65a58 ("mm: introduce MADV_COLD")
Cc: stable@vger.kernel.org
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 mm/madvise.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -426,8 +426,14 @@
 		}
 
 		if (!pageout && pmd_young(orig_pmd)) {
-			pmdp_invalidate(vma, addr, pmd);
-			orig_pmd = pmd_mkold(orig_pmd);
+			/*
+			 * Rebuild from the entry pmdp_invalidate() exchanged
+			 * out, not from orig_pmd: hardware may have set the
+			 * dirty bit since orig_pmd was read, and a MADV_FREE'd
+			 * (lazyfree) THP whose dirty bit is lost is discarded
+			 * by reclaim, silently dropping the store.
+			 */
+			orig_pmd = pmd_mkold(pmdp_invalidate(vma, addr, pmd));
 
 			set_pmd_at(mm, addr, pmd, orig_pmd);
 			tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
-- 
2.40.0