Re: [PATCH] mm/highmem: refuse to copy hwpoisoned folios in the !copy_mc fallbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mm/highmem: wrong hwpoison granularity in copy_mc_*_highpage() fallbacks
- [High] mm/highmem: copy_mc_*_highpage() reads poisoned page before flag check
--

commit e960425b93cf423ea993320abfce6ba212a1b8fe
Author: Yuanhe Shu <xiangzao@linux.alibaba.com>

mm/highmem: refuse to copy hwpoisoned folios in the !copy_mc fallbacks

This commit updates the !copy_mc_to_kernel fallbacks for
copy_mc_user_highpage() and copy_mc_highpage() to check
folio_contain_hwpoisoned_page(page_folio(from)) after copying the page
and return PAGE_SIZE if set.

Link: https://lore.kernel.org/r/877bjowf9i.fsf@DESKTOP-5N7EMDA/

> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index d7aac9de1c8a4..f90f0c6204a4e 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -474,12 +474,21 @@ static inline int copy_mc_user_highpage(struct page *to, struct page *from,
>  					unsigned long vaddr, struct vm_area_struct *vma)
>  {
>  	copy_user_highpage(to, from, vaddr, vma);
> +	/*
> +	 * The flag can be set while the copy runs, so check after it;
> +	 * a flag landing later can no longer have corrupted this copy.
> +	 */

[Severity: High]
What happens in copy_mc_user_highpage() and copy_mc_highpage() if the
source page already has PG_hwpoison set from a real hardware memory error
before the copy starts (for example, reported earlier via ACPI APEI GHES, a
patrol scrubber, or a prior user-mode fault where memory_failure() set
PG_hwpoison before locking or unmapping the folio)?

On architectures without copy_mc_to_kernel (such as arm64, riscv, and
loongarch), copy_user_highpage() and copy_highpage() map the page with
kmap_local_page(from) and perform a plain kernel memory read via
copy_user_page() or copy_page() without exception-table recovery for
hardware memory errors.

Callers such as folio_mc_copy(), wp_page_copy() -> __wp_page_copy_user(),
copy_present_page(), collapse_file(), and __collapse_huge_page_copy() rely
on copy_mc_user_highpage() and copy_mc_highpage() to refuse poisoned pages.
If the check only runs after the copy, won't reading an already-poisoned
physical page in kernel mode trigger an unrecoverable kernel-mode fault
(such as an EL1 synchronous external abort on arm64) and panic the kernel
before reaching the post-copy check?

Should both functions check whether the source page is hwpoisoned before
the copy in addition to after the copy?

> +	if (folio_contain_hwpoisoned_page(page_folio(from)))
> +		return PAGE_SIZE;
>  	return 0;
>  }
>  
>  static inline int copy_mc_highpage(struct page *to, struct page *from)
>  {
>  	copy_highpage(to, from);
> +	/* Same reasoning as in copy_mc_user_highpage(). */
> +	if (folio_contain_hwpoisoned_page(page_folio(from)))
> +		return PAGE_SIZE;

[Severity: Critical]
Is folio_contain_hwpoisoned_page(page_folio(from)) the right granularity
for single-page copy helpers like copy_mc_user_highpage() and
copy_mc_highpage()?

In include/linux/page-flags.h:

static inline bool folio_contain_hwpoisoned_page(struct folio *folio)
{
	return folio_test_hwpoison(folio) ||
	    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio));
}

Because FOLIO_PF_ANY is 0, folio_test_hwpoison(folio) only tests
PG_hwpoison on the head page (&folio->page[0]), not on a tail page passed
as from. For any tail page of a large folio,
folio_contain_hwpoisoned_page(page_folio(from)) relies purely on the
folio-wide PG_has_hwpoisoned flag. Doesn't this cause both false negatives
and false positives?

First, for tail pages with PG_hwpoison set where PG_has_hwpoisoned is not
set, folio_contain_hwpoisoned_page(page_folio(from)) returns false and the
poisoned tail page is silently copied:

1. In mm/memory-failure.c:memory_failure(), TestSetPageHWPoison(p) sets
   PG_hwpoison on the target tail page before get_hwpoison_page() and
   folio_lock(folio), while folio_set_has_hwpoisoned(folio) is only called
   after acquiring and releasing folio_lock(folio):

   memory_failure()
       if (TestSetPageHWPoison(p))
           ...
       res = get_hwpoison_page(p, flags);
       ...
       folio_lock(folio);
       ...
       folio_unlock(folio);
       if (folio_test_large(folio)) {
           ...
           folio_set_has_hwpoisoned(folio);

   Callers holding folio_lock on the source folio during the copy -- such
   as migrate_folio() / filemap_migrate_folio() / __migrate_folio() ->
   folio_mc_copy(), do_cow_fault(), collapse_file(), and
   __collapse_huge_page_copy() -- block a concurrent memory_failure() at
   folio_lock(folio) after PageHWPoison(from) is already set on the tail
   page, before folio_set_has_hwpoisoned(folio) can run.

2. If memory_failure() (e.g., via debugfs corrupt-pfn without
   MF_COUNT_INCREASED) hits a tail page while a large folio is isolated
   from the LRU for migration or khugepaged, TestSetPageHWPoison(p) sets
   PG_hwpoison on the tail page, get_hwpoison_page() fails because
   HWPoisonHandlable() returns false for !PageLRU, and memory_failure()
   aborts without ever setting PG_has_hwpoisoned.

3. When CONFIG_TRANSPARENT_HUGEPAGE=n and CONFIG_MEMORY_FAILURE=y,
   folio_test_has_hwpoisoned() is compiled as FOLIO_FLAG_FALSE (always
   false), so poisoned tail pages of large page-cache folios are never
   detected, and mf_dax_kill_procs() sets PageHWPoison(page) on FS-DAX
   subpages without setting PG_has_hwpoisoned.

Second, for a non-hugetlb large folio that has PG_has_hwpoisoned set
because one subpage is poisoned (and could not be split to order 0, e.g.
due to extra references or min_order_for_split(folio) > 0 on a large block
size filesystem), unpoisoned subpages remain PTE-mappable and accessible.
When a single-page copy runs on a healthy subpage of that folio via
do_cow_fault(), wp_page_copy() -> __wp_page_copy_user(), or
copy_present_page() during fork(),
folio_contain_hwpoisoned_page(page_folio(from)) returns true and fails the
operation with VM_FAULT_HWPOISON (SIGBUS) or -EHWPOISON even though the
copied subpage from is not poisoned.

Would using is_page_hwpoison(from) in include/linux/page-flags.h instead of
folio_contain_hwpoisoned_page(page_folio(from)) avoid both problems by
checking PageHWPoison(from) on the specific subpage being copied while
still checking the head page for hugetlb folios?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026025724.1430747-1-xiangzao@linux.alibaba.com?part=1