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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Mon, Nov 09, 2026 at 05:17:03PM +0000, Kiryl Shutsemau wrote:
> 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")

Yeah not insisting!

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

OK, though my concern is you're making an assumption based on where this is
called from, somebody else could use the function somewhere else and... :)

But this is nitpicking really, that's fine!

>
> > > +	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.

Thanks!

>
> > > @@ -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...

You could also remove the likely() in the function itself ;)

>
> --
>   Kiryl Shutsemau / Kirill A. Shutemov

--
Cheers, Lorenzo