Re: [PATCH v5 02/12] mm: Rename folio_contain_hwpoison_page() to folio_has_hwpoison_page()

[Matthew Wilcox <willy@infradead.org>]

Sashiko review:
> --- Patch [2]: [PATCH v5 02/12] mm: Rename folio_contain_hwpoison_page() to folio_has_hwpoison_page() ---
> commit 8fadf59bc3118e11dab2620f6cc376507c16b29a
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> mm: Rename folio_contain_hwpoison_page() to folio_has_hwpoison_page()
> 
> This is a better name for this predicate.  Also make the argument const
> to allow other callers to be made const.  No functional change.
> 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 7223f6f4e2b40..4185a03a45cf7 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -1083,10 +1083,10 @@ static inline bool is_page_hwpoison(const struct page *page)
> >  	return folio_test_hugetlb(folio) && PageHWPoison(&folio->page);
> >  }
> >  
> > -static inline bool folio_contain_hwpoisoned_page(struct folio *folio)
> > +static inline bool folio_has_hwpoisoned_page(const struct folio *folio)
> >  {
> >  	return folio_test_hwpoison(folio) ||
> > -	    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio));
> > +	       (folio_test_large(folio) && folio_test_has_hwpoisoned(folio));
> >  }
> 
> This isn't a bug introduced by this patch, but can KSM trigger a machine check
> exception when scanning memory?

Good lord.  I renamed a function.  Which apparently causes Sashiko to
look for bugs in every caller

> Looking at cmp_and_merge_page() in mm/ksm.c, the KSM scanner initiates page
> hash calculation:
> 
> mm/ksm.c:cmp_and_merge_page() {
>     ...
>     checksum = calc_checksum(page);
>     ...
> }
> 
> which then maps and reads the page content:
> 
> mm/ksm.c:calc_checksum() {
>     ...
>     void *addr = kmap_local_page(page);
>     checksum = xxhash(addr, PAGE_SIZE, 0);
>     ...
> }
> 
> If a page becomes hardware-poisoned before KSM scans it, shouldn't there be a
> PageHWPoison(page) or folio_has_hwpoisoned_page(folio) check before the read
> to prevent a fatal MCE?
> 
> 
> This is a pre-existing issue, but does migration risk panicking on poisoned
> memory?

Yes, lots of things risk panicking on poisoned memory.  It's never been
a priority for Linux.

> During page migration, pages are passed to __migrate_folio() which copies the
> contents without validation:
> 
> mm/migrate.c:__migrate_folio() {
>     ...
>     rc = folio_mc_copy(dst, src);
>     if (unlikely(rc))
>         return rc;
>     ...
> }
> 
> This executes the page copy in folio_mc_copy():
> 
> mm/util.c:folio_mc_copy() {
>     ...
>     if (copy_mc_highpage(folio_page(dst, i), folio_page(src, i)))
>         return -EHWPOISON;
>     ...
> }
> 
> On architectures lacking copy_mc support, this falls back to copy_highpage().
> Without a prior folio_has_hwpoisoned_page(src) check, could migrating a
> poisoned page via mbind() or move_pages() read poisoned memory and trigger a
> kernel panic?
> 
> 
> This is a pre-existing issue, but does kill_accessing_process() return the
> wrong error code when a task lacks an mm?
> 
> In memory failure handling, if the task is exiting or is a kernel thread,
> p->mm is NULL:
> 
> mm/memory-failure.c:kill_accessing_process() {
>     ...
>     if (!p->mm)
>         return -EFAULT;
>     ...
> }
> 
> Since the memory_failure() API protocol requires returning 0 for a successfully
> recovered state (no user signal needed), does returning -EFAULT cause the
> architecture MCE handler, such as kill_me_maybe(), to erroneously conclude
> recovery failed? Could this trigger unnecessary warnings and incorrectly
> attempt to force a SIGBUS on the dying task?

Dunno, and I'm done with side-quests.  Somebody else can decide if this
is a real problem that needs to be fixed.