Re: [PATCH v5] mm: memory: fix truncation removing mapped folios
From: Baolin Wang
Date: Tue Oct 27 2026 - 08:59:15 EST
On 10/27/26 6:55 PM, David Hildenbrand (Arm) wrote:
On 10/27/26 11:35, Baolin Wang wrote:
On 10/27/26 5:33 PM, David Hildenbrand (Arm) wrote:
On 10/27/26 10:03, Baolin Wang wrote:
No, keeping two pmd_none() checks cannot fix the race, as I replied before. [1]
Best to wait to submit new versions if there are open discussions? Because now
we have the same discussion in two places.
Sure.
I think you're right about the race, the issue is that we check for
pmd_is_huge(), not pmd_present() which confused me.
Clearest to me would be to not drop the second pmd_none() check but move it
where it belongs: if splitting zapped the PMD.
@@ -2101,13 +2104,15 @@ static inline unsigned long zap_pmd_range(struct
mmu_gather *tlb,
addr = next;
continue;
}
+ /* We already synchronized with concurrent zapping. */
+ if (pmd_none(*pmd)) {
+ addr = next;
+ continue;
+ }
/* fall through */
- } else if (details && details->single_folio &&
- folio_test_pmd_mappable(details->single_folio) &&
- next - addr == HPAGE_PMD_SIZE && pmd_none(*pmd)) {
- sync_with_folio_pmd_zap(tlb->mm, pmd);
- }
- if (pmd_none(*pmd)) {
+ } else if (pmd_none(*pmd)) {
+ if (details && details->single_folio)
+ sync_with_folio_pmd_zap(tlb->mm, pmd);
addr = next;
continue;
}
IMHO, I'm not a big fan of adding two pmd_none() checks just to avoid an
uncommon concurrent sync.
It's what we already had. We had to: one for handling pmd_none() directly and
one for handling zapping while splitting (file THPs).
We just managed to let the second also handle the final piece of the former.
Anyhow, let's get this fixed.
If you think the code is clearer your way (where I disagree)
Fair enough. Though I have a different taste, I'd still like to make the maintainer happy. I'll send out v6 tomorrow following your suggestion.
But could you help check the following whole diff to avoid any unnecessary misunderstandings? (I don't want to give Andrew more trouble :)). Thanks.
diff --git a/mm/internal.h b/mm/internal.h
index b1b0f29d2216..ffb8eaec90e3 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -584,14 +584,16 @@ void free_pgtables(struct mmu_gather *tlb, struct unmap_desc *desc);
void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte);
/**
- * sync_with_folio_pmd_zap - sync with concurrent zapping of a folio PMD
+ * sync_with_folio_pmd_zap - sync with concurrent zapping of folios
* @mm: The mm_struct.
* @pmdp: Pointer to the pmd that was found to be pmd_none().
*
* When we find a pmd_none() while unmapping a folio without holding the PTL,
- * zap_huge_pmd() may have cleared the PMD but not yet modified the folio to
- * indicate that it's unmapped. Skipping the PMD without synchronization could
- * make folio unmapping code assume that unmapping failed.
+ * zap_huge_pmd() may have cleared the PMD but not yet updated the folio
+ * mapcount, and zap_empty_pte_table() clears the PMD only after the rmaps
+ * for the table have been removed, but nothing orders the pmd_none() read
+ * against later mapcount reads. Skipping the PMD without synchronization
+ * could make folio unmapping code assume that unmapping failed.
*
* Wait for concurrent zapping to complete by grabbing the PTL.
*/
diff --git a/mm/memory.c b/mm/memory.c
index e911e63ba1ed..e0fef107a022 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2036,6 +2036,15 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
}
} while (pte += nr, addr += PAGE_SIZE * nr, addr != end);
+ add_mm_rss_vec(mm, rss);
+ lazy_mmu_mode_disable();
+
+ /* Do the actual TLB flush before dropping ptl */
+ if (force_flush) {
+ tlb_flush_mmu_tlbonly(tlb);
+ tlb_flush_rmaps(tlb, vma);
+ }
+
/*
* Fast path: try to hold the pmd lock and unmap the PTE page.
*
@@ -2043,18 +2052,12 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
* to hold the pmd lock failed, then we need to recheck all pte entries
* to ensure they are still none, thereby preventing the pte entries
* from being repopulated by another thread.
+ *
+ * Clear the PMD after the rmaps for the table have been removed, in case
+ * an incorrect mapcount is observed after unmapping the folio.
*/
if (can_reclaim_pt && direct_reclaim && addr == end)
direct_reclaim = zap_empty_pte_table(mm, pmd, ptl, &pmdval);
-
- add_mm_rss_vec(mm, rss);
- lazy_mmu_mode_disable();
-
- /* Do the actual TLB flush before dropping ptl */
- if (force_flush) {
- tlb_flush_mmu_tlbonly(tlb);
- tlb_flush_rmaps(tlb, vma);
- }
pte_unmap_unlock(start_pte, ptl);
/*
@@ -2101,13 +2104,15 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
addr = next;
continue;
}
+ /* We already synchronized with concurrent zapping. */
+ if (pmd_none(*pmd)) {
+ addr = next;
+ continue;
+ }
/* fall through */
- } else if (details && details->single_folio &&
- folio_test_pmd_mappable(details->single_folio) &&
- next - addr == HPAGE_PMD_SIZE && pmd_none(*pmd)) {
- sync_with_folio_pmd_zap(tlb->mm, pmd);
- }
- if (pmd_none(*pmd)) {
+ } else if (pmd_none(*pmd)) {
+ if (details && details->single_folio)
+ sync_with_folio_pmd_zap(tlb->mm, pmd);
addr = next;
continue;
}