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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] mm/memory: PG_uptodate check in do_shared_fault() runs after
  ->page_mkwrite()
--

commit 700a620ccdd59b966f387e178bdcded1ac07e9a0
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

mm/memory: recheck PG_uptodate after ->page_mkwrite()

This commit rechecks whether a file folio is still uptodate in
do_shared_fault() after ->page_mkwrite() returns with the folio locked,
retrying the fault if PG_uptodate was cleared while the folio was unlocked.

> diff --git a/mm/memory.c b/mm/memory.c
> index fe72e66c9c2ee..cc1d0b7d15c75 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c

[ ... ]

> @@ -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))) {
> +			folio_unlock(folio);
> +			folio_put(folio);
> +			return VM_FAULT_NOPAGE;
> +		}
>  	}
>  
>  	ret |= finish_fault(vmf);

[Severity: High]
Does checking file_folio_not_uptodate(folio) after do_page_mkwrite() run too
late once ->page_mkwrite() has already dirtied the folio or set PG_uptodate
back on it?

In the race with ecryptfs_writepages(), writeback clears PG_dirty via
folio_clear_dirty_for_io() and then clears PG_uptodate when encryption fails.
When filemap_page_mkwrite() runs next, it relocks the folio, checks only
folio->mapping != mapping, and calls folio_mark_dirty():

mm/filemap.c:filemap_page_mkwrite() {
	...
	folio_lock(folio);
	if (folio->mapping != mapping) {
		folio_unlock(folio);
		ret = VM_FAULT_NOPAGE;
		goto out;
	}
	/*
	 * We mark the folio dirty already here so that when freeze is in
	 * progress, we are guaranteed that writeback during freezing will
	 * see the dirty folio and writeprotect it again.
	 */
	folio_mark_dirty(folio);
	...
}

Before returning to do_shared_fault(), folio_mark_dirty() ->
__folio_mark_dirty() triggers WARN_ON_ONCE(warn && !folio_test_uptodate(folio))
and tags the non-uptodate folio PAGECACHE_TAG_DIRTY.  When do_shared_fault()
then aborts with VM_FAULT_NOPAGE, the non-uptodate folio is left unlocked and
dirty in the page cache, where writeback can pick it up again or reclaim
cannot evict it.

On filesystems whose ->page_mkwrite() goes through block_page_mkwrite() or
btrfs_page_mkwrite(), block_commit_write() and btrfs_folio_set_uptodate() set
PG_uptodate back on a full folio without reading from disk, so
file_folio_not_uptodate(folio) returns false and the invalidated folio is
still mapped into userspace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109120403.82361-1-kas@kernel.org?part=1