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

[Lorenzo Stoakes (ARM)]

On Sun, Oct 11, 2026 at 06:22:27AM -0700, samarth.colleges@xxxxxxxxx wrote:
> From: Samarth Narang <samarth.colleges@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Samarth Narang <samarth.colleges@xxxxxxxxx>

Thanks for the report.

As this seems to be AI-generated, a member of the core team will take over
this.

> ---
>  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

--
Cheers, Lorenzo