Re: [PATCH v2 1/2] mm/memory: recheck PG_uptodate after ->page_mkwrite()

[Kiryl Shutsemau <kas@kernel.org>]

On Mon, Nov 09, 2026 at 04:11:40PM +0000, Lorenzo Stoakes (ARM) wrote:
> On Mon, Nov 09, 2026 at 12:04:01PM +0000, Kiryl Shutsemau (Meta) wrote:
> > do_shared_fault() unlocks the folio around ->page_mkwrite(). Writeback
> > can lock it in that window, fail, and clear PG_uptodate with the folio
> > left in the page cache. ecryptfs_writepages() does this when encryption
> > fails. filemap_page_mkwrite() relocks the folio and checks only
> > ->mapping, so the fault maps a folio that is not uptodate:
> >
> >     CPU0                            CPU1
> >     do_shared_fault()
> >       folio_unlock()
> >                                     ecryptfs_writepages()
> >                                       folio_clear_uptodate()
> >       filemap_page_mkwrite()
> >         folio_lock()
> >       finish_fault()
> >
> > The next read(2) or fault on the folio reads it in again from the file,
> > over anything stored through the mapping in the meantime. Those stores
> > are lost.
> >
> > Recheck PG_uptodate once ->page_mkwrite() returns with the folio locked,
> > and retry the fault if it is clear, the same way filemap_fault() retries
> > on a cached folio that lost PG_uptodate. Skip folios outside the page
> > cache: driver pages from ->fault() never get PG_uptodate and would
> > retry forever.
> >
> > The check stays out of do_page_mkwrite(): wp_page_shared() calls it on
> > a folio that is still mapped, and a retry there would fault on the same
> > PTE forever.
> >
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Closes: https://lore.kernel.org/sashiko-outbox-202966@kernel.org/
> 
> No Fixes? Sorry I may have missed context from v1 here :)

The window is from 2007, when __do_fault() started to drop the folio
lock around ->page_mkwrite():

Fixes: 6967614761fd ("ocfs2: release page lock before calling ->page_mkwrite")

Not sure how useful such Fixes is. It predates any possible stable tree.

> > ---
> >  mm/memory.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index fe72e66c9c2e..cc1d0b7d15c7 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -5613,6 +5613,18 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
> >  	return VM_FAULT_OOM;
> >  }
> >
> > +/*
> > + * A page cache folio that is not uptodate holds data that was never read in
> > + * or cleared, and mapping it leaks stale memory to userspace.
> > + */
> > +static bool file_folio_not_uptodate(struct folio *folio)
> > +{
> > +	if (likely(folio_test_uptodate(folio)))
> > +		return false;
> > +	/* Driver pages from ->fault() never get PG_uptodate */
> 
> I guess you mean drivers that return non-page cache folios from ->fault()?

Yes. How about:

        /*
         * ->fault() can also return driver memory that is not in the page
         * cache. It never gets PG_uptodate.
         */

I would rather avoid "rmappable". These folios do get
folio_add_file_rmap_ptes() in set_pte_range(), and the word reads like
PG_large_rmappable. As you said, DAX never reaches the check, so "page
cache" is accurate.

> > +	return !folio_test_anon(folio) && folio->mapping;
> 
> Technically folio_test_anon() can never return true based on where this is
> called from right? Could this just be a:
> 
> 	VM_WARN_ON_ONCE(folio_test_anon(folio));
> 	/* ... comment */
> 	return folio->mapping;
> 
> Instead?

Right, all callers pass a file folio.

I will make it an assert, but at the top of the helper, so it checks
every call and not only the rare !uptodate one.

> > @@ -6163,6 +6175,16 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
> >  			folio_put(folio);
> >  			return tmp;
> >  		}
> > +
> > +		/*
> > +		 * The folio was unlocked for ->page_mkwrite(). If it lost
> > +		 * PG_uptodate meanwhile, retry the fault to read it in again.
> > +		 */
> > +		if (unlikely(file_folio_not_uptodate(folio))) {
> 
> As Pedro said, I think this unlikely() is probably unnecessary given the true
> return case is implicitly unlikely.

As I told Pedro, I prefer it at the call site. That is where the reader
needs to see that the retry is the rare path. But if you insist...

-- 
  Kiryl Shutsemau / Kirill A. Shutemov