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

["Huang, Ying" <ying.huang@linux.alibaba.com>]

Yuanhe Shu <xiangzao@linux.alibaba.com> writes:

> Where copy_mc_to_kernel is defined, copy_mc_user_highpage() and
> copy_mc_highpage() stop at a hardware error during the copy, report
> the not-copied bytes and queue the source page for memory_failure().
> The !copy_mc fallbacks return 0 after a plain copy instead.  With
> PG_hwpoison set while the data stays readable - software-injected
> poison (MADV_HWPOISON, the debugfs corrupt-pfn injector) or a
> transient hardware error - there is no fault to recover from: the
> copy completes, the destination folio carries no poison marker, and
> the poison is consumed silently, the same poison consumption
> commit 841a8bfcbad9 ("mm: prevent poison consumption when splitting
> THP") refused to risk on the split path.

As far as I know, software-injected poison is used for debugging only ,
so I don't think it's that important.  Hardware poison state in DDR is
sticky; that is, even a transient hardware error will cause a permanent
hardware poison state until a reset.

Therefore, the primary value of this patch should be to reduce machine
checks (and panics) during memory copies.  If so, we should place the
check code before copying.

> Check the source folio with folio_contain_hwpoisoned_page() in both
> fallbacks and return PAGE_SIZE, the not-copied-bytes convention of
> the real copy_mc paths, so the callers take their existing error
> paths: the COW and fork paths fail with -EHWPOISON, KSM and
> khugepaged abort, and folio_mc_copy() propagates to the migration
> core, where this composes with the separate migration-side checks.
> On archs with copy_mc this code does not compile.
>
> The check runs after the copy: a check before it would leave open
> the window between itself and the copy.  A flag set before or during
> the copy is caught, whatever state the data is in.  A late software
> injection cannot have corrupted the completed copy, and a synchronous
> hardware error takes an arch without copy_mc down at the access; the
> residue no flag check, before or after, can catch is corruption whose
> hardware report lands after the last check.  Closing that would need
> architecture support, as discussed.
>
> The fallbacks do not queue memory_failure() on refusal: the flag is
> already set, and memory failure either knows about the page or has
> abandoned it.
>
> Suggested-by: "Huang, Ying" <ying.huang@linux.alibaba.com>
> Suggested-by: Kaitao Cheng <kaitao.cheng@linux.dev>
> Assisted-by: LLM
> Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
> Link: https://lore.kernel.org/r/877bjowf9i.fsf@DESKTOP-5N7EMDA/
> ---
>  include/linux/highmem.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index d7aac9de1c8a..f90f0c6204a4 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.
> +	 */
> +	if (folio_contain_hwpoisoned_page(page_folio(from)))
> +		return PAGE_SIZE;

Why not just use PageHWPoison()?

>  	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;
>  	return 0;
>  }
>  #endif

---
Best Regards,
Huang, Ying