Re: [PATCH] mm/shmem: use invalidate_lock to fix hole-punch race

[Gregory Price <gourry@gourry.net>]

On Thu, Mar 26, 2026 at 05:07:42PM +0000, Pedro Falcato wrote:
> > Two races allow PTEs to be re-installed for a folio that fallocate
> > is about to remove from page cache:
> 
> Hmm, I don't see how your patch fixes anything.
> 

after looking at your comments below i realized race 2 actually requires
the fork as well, which means they're both essentially variations of the
same race, so hopefully i can simplify the change log.

> >   fallocate              fault-around           fork
> >   --------               ------------           ----
> >   set i_private
> >   unmap_mapping_range()
> >   # zaps PTEs
> >                        filemap_map_pages()
> >                         # re-maps folio!
> >                                               dup_mmap()
> >                                               # child VMA
> >                                               # in tree
> >   shmem_undo_range()
> >     lock folio
> >     unmap_mapping_folio()
                  ^^^ i_mmap_lock_read held, iterates VMAs
> 	spin_lock(ptl);
                  ^^^ child VMA's PTL
> >     # child VMA:
> >     #   no PTE, skip
> 	spin_unlock(ptl);
                    ^^^ child VMA done, iterator moves on
		        it will not re-visit the child.

> >                                             copy_page_range()
>                                                spin_lock(dst_ptl);
                                                   ^ Child PTL
> 					       spin_lock(src_ptl);
                                                   ^ Parent PTL
> 						/* does not copy PTE. either
> 						 * we find a zapped PTE, or unmap_mapping_folio()
> 						 * finds two mappings instead of one. */

At this point, unmap_mapping_folio only processed the child VMA
(no PTE, skip). The parent PTE *has not* been zapped.

copy_page_range() acquires src_ptl (parent) and reads a present PTE,
and boom copies it to child.

When it reaches the parent VMA next, it zaps the parent PTE,
but the child PTE (just installed) survives.  

> > 
> > Fix both races with invalidate_lock.
> > 
> 
> I don't see what you're seeing? Note that both map_pages and fault()
> take the folio lock (map_pages does a trylock) to exclude against truncate
> as well.
> 

The folio lock serializes map_pages/fault against truncate - but the
race isn't between those two. It's between truncate's unmap walk and
fork's copy_page_range - and copy_page_range doesn't take folio lock.

The easiest way to deal with this is to prevent these fork-inserted PTEs
from existing rather than try to make copy_page_range aware of
truncation (it already holds the PTL when it finds the PTE, so you can't
take the folio lock unless you drop/reacquire the PTL).

~Gregory