Re: [BUG] shmem: FALLOC_FL_PUNCH_HOLE vs fault-around race corrupts page cache / rss counters

[Pedro Falcato <pfalcato@suse.de>]

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@modal.com> 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@linux-foundation.org>
> > > 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.

> 
>         /*
> @@ -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?

-- 
Pedro