Re: [PATCH] mm/memory: refuse to map page cache folios that are not uptodate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm: user-triggerable WARN_ONCE in do_set_pmd() via
  MADV_COLLAPSE on fallocated shmem THP
- [Medium] mm: missing folio_test_uptodate() recheck in
  do_shared_fault() mkwrite path
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] mm/khugepaged: leak of vmf.prealloc_pte in set_huge_pmd() on
  fallback [1]
--

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

mm/memory: refuse to map page cache folios that are not uptodate

This commit adds file_folio_not_uptodate() to warn and return VM_FAULT_SIGBUS
in do_set_pmd() and finish_fault() if a page cache folio is mapped without
PG_uptodate set.

> diff --git a/mm/memory.c b/mm/memory.c
> index 6fe0b7dab927c..ff2f2deff761e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5674,6 +5674,22 @@ static vm_fault_t __do_fault(struct vm_fault *vmf)
>  	return ret;
>  }
>  
> +/*
> + * 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. The callers are
> + * expected to check, so treat a hit as a kernel bug and refuse the mapping.
> + */
> +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 */
> +	if (folio_test_anon(folio) || !folio->mapping)
> +		return false;
> +	WARN_ONCE(1, "refusing to map a page cache folio that is not uptodate\n");
> +	return true;
> +}
> +
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  static void deposit_prealloc_pte(struct vm_fault *vmf)
>  {
> @@ -5714,6 +5730,9 @@ vm_fault_t do_set_pmd(struct vm_fault *vmf, struct folio *folio, struct page *pa
>  		return ret;
>  	page = &folio->page;
>  
> +	if (file_folio_not_uptodate(folio))
> +		return VM_FAULT_SIGBUS;
> +

[Severity: Medium]
Can madvise(MADV_COLLAPSE) on an unmapped, fallocated PMD-order shmem folio
still reach do_set_pmd() and trigger the WARN_ONCE() in
file_folio_not_uptodate()?

When shmem_fallocate() allocates a PMD-order folio via shmem_get_folio_gfp()
with SGP_FALLOC, it leaves PG_uptodate clear until first use.  During
MADV_COLLAPSE, collapse_scan_file() sees is_pmd_order(folio_order(folio)) and
returns SCAN_PTE_MAPPED_HUGEPAGE without checking folio_test_uptodate(folio):

mm/khugepaged.c:collapse_scan_file() {
    ...
		if (is_pmd_order(folio_order(folio))) {
			result = SCAN_PTE_MAPPED_HUGEPAGE;
			failed_pfn = folio_pfn(folio);
			/*
			 * PMD-sized THP implies that we can only try
			 * retracting the PTE table.
			 */
			folio_put(folio);
			break;
		}
    ...
}

collapse_pmd() then skips collapse_file() and calls
try_collapse_pte_mapped_thp(), which locks the folio and calls set_huge_pmd()
-> do_set_pmd() without checking folio_test_uptodate(folio) either.  An
unprivileged user on a tmpfs mount with huge pages enabled can trigger this
WARN_ONCE() and cause MADV_COLLAPSE to fail with -EINVAL.

[Severity: High]
This problem wasn't introduced by this patch, but can set_huge_pmd() leak
vmf.prealloc_pte [1] when do_set_pmd() falls back after allocating it?

On architectures where arch_needs_pgtable_deposit() is true, do_set_pmd()
allocates vmf->prealloc_pte via pte_alloc_one() before taking pmd_lock(), and
returns VM_FAULT_FALLBACK if *vmf->pmd is no longer pmd_none():

mm/memory.c:do_set_pmd() {
    ...
	if (arch_needs_pgtable_deposit() && !vmf->prealloc_pte) {
		vmf->prealloc_pte = pte_alloc_one(vma->vm_mm);
		if (!vmf->prealloc_pte)
			return VM_FAULT_OOM;
	}

	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
	if (unlikely(!pmd_none(*vmf->pmd)))
		goto out;
    ...
}

If a concurrent operation that does not take the folio lock (such as
madvise_guard_install() or mfill_atomic() on a private VMA) populates the PMD
before pmd_lock(), do_set_pmd() returns VM_FAULT_FALLBACK with
vmf.prealloc_pte still allocated, and set_huge_pmd() returns SCAN_FAIL without
calling pte_free().

>  	/*
>  	 * Just backoff if any subpage of a THP is corrupted otherwise
>  	 * the corrupted page may mapped by PMD silently to escape the
> @@ -5849,6 +5868,16 @@ vm_fault_t finish_fault(struct vm_fault *vmf)
>  	unsigned long addr;
>  	bool needs_fallback = false;
>  
> +	/*
> +	 * Check the folio ->fault() returned. For CoW that is the source of the
> +	 * copy; DAX CoW has none.
> +	 */
> +	if (vmf->page) {
> +		folio = page_folio(vmf->page);
> +		if (file_folio_not_uptodate(folio))
> +			return VM_FAULT_SIGBUS;
> +	}

[Severity: Medium]
Can a shared writable fault hit the WARN_ONCE() and VM_FAULT_SIGBUS in
finish_fault() if PG_uptodate is cleared while the folio is unlocked for
->page_mkwrite()?

In do_shared_fault(), __do_fault() returns a locked uptodate folio, which is
then unlocked before calling do_page_mkwrite().  While the folio lock is
dropped, writeback error handling on filesystems such as ext4
(mpage_release_unused_pages()), ecryptfs (ecryptfs_writepages()), or ntfs
(ntfs_compress_write()) can call folio_clear_uptodate() without removing the
folio from folio->mapping:

CPU0                                    CPU1
do_shared_fault()
  __do_fault()
  folio_unlock(folio)
                                        ext4_do_writepages()
                                          mpage_map_and_submit_extent()
                                            mpage_release_unused_pages()
                                              folio_clear_uptodate(folio)
  do_page_mkwrite()
    iomap_page_mkwrite()
      folio_lock(folio)
      return VM_FAULT_LOCKED
  finish_fault()
    file_folio_not_uptodate(folio)      <-- WARN_ONCE + VM_FAULT_SIGBUS

Unlike f2fs_vm_page_mkwrite() or gfs2_page_mkwrite(), handlers such as
filemap_page_mkwrite(), iomap_page_mkwrite(), and block_page_mkwrite() (as
well as do_page_mkwrite() itself) re-lock the folio and check folio->mapping,
but do not recheck folio_test_uptodate(folio) to retry the fault with
VM_FAULT_NOPAGE.

[1] https://sashiko.dev/#/bug/linux-d0498ac5-724c-4edc-8408-65bd846cca7a

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106144323.3634477-1-kirill@shutemov.name?part=1