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

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 8 Oct 2026 11:24:43 +0200 Jan Kara <jack@suse.cz> wrote:

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

back to the drawing board...


You're right.  I pushed this further and I no longer think the explanation
in the changelog is sufficient.

filemap_map_pages() takes the folio lock, and shmem_undo_range() also locks
the folio before truncating it.  truncate_inode_folio() then calls
truncate_cleanup_folio() while that lock is held, and
truncate_cleanup_folio() calls unmap_mapping_folio() before
filemap_remove_folio().

So if fault-around maps the folio first, truncation should subsequently
unmap it.  If truncation gets the folio lock first, fault-around cannot map
it before it is removed from the page cache.  I don't see a direct
fault-around-only race which leaves the folio mapped at
filemap_remove_folio().

The earlier fork/copy_page_range() theory did have a way around the folio
lock, since copy_page_range() does not take it.  But Ayush's single-process
reproducer rules that explanation out.

The strongest remaining clue is that the reproducer requires repeated
partial splitting and aggressive khugepaged re-collapse, and the production
splat was on an order-3 folio with nonzero mapcount.  That makes a
split/re-collapse/rmap or mapcount problem look more likely than the simple
fault-around race I described.

So I don't think this patch is ready as-is.  It might suppress the problem by
adding broader serialization, but that would not demonstrate that it fixes
the underlying bug.

A useful next step would be to instrument truncate_cleanup_folio(), e.g. warn
immediately after unmap_mapping_folio() if folio_mapped() is still true.
That should distinguish "unmap failed to remove an existing mapping" from
"some path installed a new mapping afterwards", and narrow this down
considerably.