Re: [BUG] shmem: FALLOC_FL_PUNCH_HOLE vs fault-around race corrupts page cache / rss counters
From: Baolin Wang
Date: Fri Oct 09 2026 - 05:20:38 EST
On 10/9/26 4:41 PM, Pedro Falcato wrote:
On Fri, Oct 09, 2026 at 02:46:18PM +0800, Baolin Wang wrote:
On 10/8/26 5:24 PM, Jan Kara wrote:
On Sun 04-10-26 22:28:31, Andrew Morton wrote:
On Sat, 3 Oct 2026 03:31:03 +0000 Ayush Ranjan <ayushr@xxxxxxxxx> wrote:
Gentle ping on this. The reproducer in my previous mail [1] triggers
"Bad page cache ... still mapped when deleted" on 6.18.46 within a
couple of minutes on a 128-CPU bare-metal box, with no fork() and no
gVisor involved.
We continue to hit this in production at low frequency, so I am happy
to test patches or collect more data if that would help.
fwiw I made gpt and gemini argue about this for a while and ended up
with the below.
Worth a try I guess. Let's see :)
From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Subject: mm: shmem: serialize fault-around against hole punching
Date: Sun Oct 4 09:05:31 PM PDT 2026
shmem uses filemap_map_pages() for fault-around. Unlike shmem_fault(),
filemap_map_pages() does not participate in shmem's fallocate exclusion
protocol.
During a partial hole punch of a large shmem folio, the folio can be split
and the resulting smaller folios can remain temporarily visible in the page
cache while shmem_undo_range() restarts its walk. Fault-around can then map
one of those folios again before the hole-punch path removes it.
shmem_undo_range() may subsequently delete that folio from the page cache
despite the new userspace mapping. This can trigger "still mapped when
deleted" warnings and leave stale mappings or inconsistent RSS/page-table
accounting behind.
So this part of explanation is either incomplete or wrong in my opinion.
Yes, shmem_undo_range() calls truncate_inode_partial_folio() which can
split a large folio. Yes, filemap_map_pages() can map those pages back into
page tables. But how "shmem_undo_range() may subsequently delete that folio
from the page cache despite the new userspace mapping" happens is unclear
to me. After splitting a folio, shmem_undo_range() will restart and find the
newly split (and mapped) folios and calls truncate_inode_folio() to get rid
of them. Now truncate_inode_folio() calls truncate_cleanup_folio() which
does:
if (folio_mapped(folio))
unmap_mapping_folio(folio);
so the mapping is reliably removed under folio lock. Can you perhaps push
your agents further to explain in more detail how this "still mapped when
deleted" happens in their opinion?
Good point and I think you are right.
Yesterday I quickly reproduced the issue with Ayush's reproducer, and I got
the following crash info. From the dump message, we can see that
truncate_inode_folio() is really trying to remove mapped folios, which is
incorrect.
I also quickly tried Andrew's patch, and the issue no longer reproduces, so
I initially thought that was the root cause. But after your reminder, I now
believe Andrew's patch merely workaround the issue rather than fixing the
actual root cause.
Today I'm going to re-analyze the race with the reproducer (thanks Ayush).
After analysis, I believe the race exists between truncation and
MADV_DONTNEED, and shmem's fault_around() merely makes the issue easier to
reproduce. Since MADV_DONTNEED synchronously releases the pagetable page
before calling tlb_flush_rmaps(), this could cause another thread's
truncation to skip zap_pte_range() but still observe the folio's mapcount as
non-zero. A possible race scenario is as follows:
CPU 0 CPU 1
madvise_dontneed_single_vma
shmem_fallocate ......
...... zap_pte_range
truncate_inode_folio zap_empty_pte_table(pmd clear)
unmap_mapping_folio
......
zap_pmd_range(saw pmd none)
filemap_remove_folio
BUG_ON(folio_mapped)
tlb_flush_rmaps
Thanks for the investigation!
So, I think I understand the problem (rmap walks race PTE zapping, which no
longer serializes on the PTE lock), but I don't understand your solution at
all.
Based on the above race analysis, I made the following fix that uses the PMD
lock synchronously to prevent this race, and the issue no longer reproduces.
I will clean it up and send out a formal patch.
diff --git a/mm/memory.c b/mm/memory.c
index 6a8e7772b8d6..2039ada99b64 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.
*
@@ -2046,15 +2055,6 @@ static unsigned long zap_pte_range(struct mmu_gather
*tlb,
*/
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);
Namely, why moving this hunk of code up there makes any difference.
I suppose it's subtly changing memory order, but it's not immediate
in any way why this is correct.
What you really need (I think) is a happens-before relationship between
the rmap changes and the PMD zapping. Something like:
diff --git a/mm/memory.c b/mm/memory.c
index 330cde31bf8b..17cdd48fff27 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2108,6 +2108,13 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
sync_with_folio_pmd_zap(tlb->mm, pmd);
}
if (pmd_none(*pmd)) {
+ /*
+ * Pairs with tlb_flush_rmaps() in PTE zapping.
+ * Possibly skipping a page table (due to PTE zapping)
+ * needs to enforce ordering between the rmap changes
+ * and the PMD getting cleared.
+ */
+ smp_rmb();
addr = next;
continue;
}
diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c
index 9f353f0e2ef4..47b25c42bf77 100644
--- a/mm/mmu_gather.c
+++ b/mm/mmu_gather.c
@@ -89,6 +89,10 @@ void tlb_flush_rmaps(struct mmu_gather *tlb, struct vm_area_struct *vma)
tlb_flush_rmap_batch(&tlb->local, vma);
if (tlb->active != &tlb->local)
tlb_flush_rmap_batch(tlb->active, vma);
+ /*
+ * rmap changes need to be observed before e.g PTEs get zapped.
+ */
+ smp_wmb();
tlb->delayed_rmap = 0;
}
#endif
On top of your diff. Where the smp_wmb() is perhaps not required: I suppose TLB flushing
works as a sort of memory barrier itself on most/all architectures.
Either way, we have to document and fix expectations around this code.
Agree, I don't think we need smp_wmb() here. Moreover, zap_empty_pte_table() will call the pmd lock/unlock, which already indicates a memory barrier.
/*
@@ -2103,7 +2103,6 @@ static inline unsigned long zap_pmd_range(struct
mmu_gather *tlb,
}
/* fall through */
} else if (details && details->single_folio &&
- folio_test_pmd_mappable(details->single_folio) &&
Why?
Ah, sorry, this change can be removed.