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

[Pedro Falcato]

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@xxxxxxxxxx>
> Closes: https://lore.kernel.org/sashiko-outbox-202966@xxxxxxxxxx/
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> ---
>  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 */
> +	return !folio_test_anon(folio) && folio->mapping;

FYI you can wrap this line in an unlikely() and it has the same effect as
wrapping every call:
	return unlikely(!folio_test_anon(folio) && folio->mapping);

> +}
> +
>  /*
>   * Either the VMA lock or the mmap_lock must have been held on entry
>   * (see FAULT_FLAG_VMA_LOCK) and may have been released depending on
> @@ -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.
> +		 */

The "read it in again" sounds a bit too prescriptive but, apart from that:

Reviewed-by: Pedro Falcato <pfalcato@xxxxxxx>

Thanks!

-- 
Pedro