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 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 :)

> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> ---
>  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()?
Because the driver could return page cache folios there (e.g. shmem), in
which case folio->mapping would be set (set on being added to the page
cache).

But also things are complicated by DAX (of course) which _does_ set
folio->mapping but installs the folio itself so won't reach the test
anyway.

So I think 'rmappable' is the better thing to say given DAX and, given this
is fiddly, maybe worth explicitly pointing out that ->fault might return
non-rmappable folios, e.g.:

	/*
	 * Drivers which specify ->fault() might return non-rmappable folios
	 * (in which case no PG_uptodate).
	 */

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

(Note that __folio_add_file_rmap() asserts on folio_test_anon(folio)).

> +}
> +
>  /*
>   * 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.
> +		 */
> +		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.

> +			folio_unlock(folio);
> +			folio_put(folio);
> +			return VM_FAULT_NOPAGE;
> +		}
>  	}
>
>  	ret |= finish_fault(vmf);
> --
> 2.54.0
>

--
Cheers, Lorenzo