Re: [PATCH v3] mm/memory-failure: set PG_hwpoison of a non-hugetlb page under hugetlb_lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mm/memory-failure: missed retry in memory_failure() on hugetlb
  allocation
- [High] mm/memory-failure: first vmemmap page HVO race in
  clear_non_hugetlb_hwpoison()
- [Medium] mm/memory-failure: stale tail PG_hwpoison in
  clear_non_hugetlb_hwpoison()
--

commit 473f986d60ad157a360960de8cae3064b0d63305
Author: Mani Tofigh <mt4022@columbia.edu>

mm/memory-failure: set PG_hwpoison of a non-hugetlb page under hugetlb_lock

This commit sets PG_hugetlb on new hugetlb folios and sets PG_hwpoison on
non-hugetlb pages under hugetlb_lock to prevent racing with HugeTLB vmemmap
optimization (HVO), and adds a helper to conditionally clear PG_hwpoison on
the memory_failure() retry path.

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 78f0a13349cb3..9795f07b26f59 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
[ ... ]
> @@ -2170,10 +2179,32 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags)
>  	return identify_page_state(pfn, p, page_flags);
>  }
>  
> +/*
> + * If the page has become part of a hugetlb folio in the meantime, HVO may
> + * remap its struct page unless it is in the first vmemmap page of the
> + * folio.  Keep the flag then, since the retry takes it as its own.
> + */
> +static void clear_non_hugetlb_hwpoison(struct page *page)
> +{
> +	struct folio *folio;
> +
> +	spin_lock_irq(&hugetlb_lock);
> +	folio = page_folio(page);
> +	if (!folio_test_hugetlb(folio) ||
> +	    folio_page_idx(folio, page) < VMEMMAP_OPTIMIZATION_NR_STRUCT_PAGES)
> +		ClearPageHWPoison(page);

[Severity: High]
Does HVO also remap the first vmemmap page of a hugetlb folio without holding
hugetlb_lock?

In vmemmap_optimize_pte(), when vmemmap_optimizable_pfn() is false (which is
the first vmemmap page of the folio), HVO duplicates src into a newly
allocated page dst, remaps the PTE to dst, and frees src:

mm/hugetlb_vmemmap.c:vmemmap_optimize_pte() {
	if (likely(vmemmap_optimizable_pfn(page_to_pfn(page), folio_order(folio)))) {
		dst = folio_vmemmap_shared_tail_page(folio);
		if (!dst)
			return -ENOMEM;
	} else {
		dst = vmemmap_dup_page_from_list(src, walk->vmemmap_pages);
	}

	list_add(&src->lru, walk->vmemmap_pages);
	...
	set_pte_at(&init_mm, addr, pte, mk_pte(dst, PAGE_KERNEL));
}

Because init_new_hugetlb_folio() sets PG_hugetlb before HVO runs, this creates
a race window on the first vmemmap page:

CPU0 (memory_failure)             CPU1 (hugetlb allocation)
get_huge_page_for_hwpoison()
  TestSetPageHWPoison(page)
                                  init_new_hugetlb_folio()
                                    __folio_set_hugetlb(folio)
                                  vmemmap_optimize_pte()
                                    dst = vmemmap_dup_page_from_list(src, ...)
                                      copy_page(dst, src)
clear_non_hugetlb_hwpoison()
  ClearPageHWPoison(page)  <-- clears old src page
                                    set_pte_at(..., dst, ...)
                                    flush_tlb_all()

After the remap, dst still has PG_hwpoison set.  If page is the head page
(index 0), the retry in get_huge_page_for_hwpoison() ->
hugetlb_update_hwpoison() sees folio_test_set_hwpoison(folio) is already true
on dst and aborts recovery with -EHWPOISON (MF_FAILED), leaving the poisoned
hugetlb folio undissolved in the pool or mapped in user processes with an
empty raw_hwp_page list.  Conversely, if hugetlb_update_hwpoison() runs
between copy_page() and set_pte_at()/flush_tlb_all(), its writes to
folio->flags and folio->_hugetlb_hwpoison (at struct page index 3, also in
the first vmemmap page) land in the displaced src page and are lost when src
is freed.

> +	spin_unlock_irq(&hugetlb_lock);
> +}

[Severity: Medium]
When folio_test_hugetlb(folio) is true and folio_page_idx(folio, page) is at
least VMEMMAP_OPTIMIZATION_NR_STRUCT_PAGES, can skipping ClearPageHWPoison()
leave a stale PG_hwpoison bit on a tail struct page?

When HVO is disabled, unsupported for the hstate, fails, or has not run yet,
the tail struct page is not remapped.  On the retry path,
try_memory_failure_hugetlb() -> get_huge_page_for_hwpoison() ->
hugetlb_update_hwpoison() sets PG_hwpoison on the head page and adds page to
raw_hwp_list_head(folio), but never clears PG_hwpoison on the tail struct
page.

If unpoison_memory() is later called on that tail PFN while the hugetlb folio
is in use (ghp > 0), it frees raw_hwp_list_head(folio), drops the ghp
reference, and then TestClearPageHWPoison(p) succeeds on the tail page and
calls folio_put(folio) a second time, dropping the active reference on the
in-use hugetlb folio while leaving PG_hwpoison set on the head page:

mm/memory-failure.c:unpoison_memory() {
	} else {
		if (folio_test_hugetlb(folio)) {
			huge = true;
			count = folio_free_raw_hwp(folio, false);
			if (count == 0) {
				folio_put(folio);
				goto unlock_mutex;
			}
		}

		folio_put(folio);
		if (TestClearPageHWPoison(p)) {
			folio_put(folio);
			ret = 0;
		}
	}
}

[ ... ]
> @@ -2487,7 +2520,7 @@ int memory_failure(unsigned long pfn, int flags)
>  			} else {
>  				/* We lost the race, try again */
>  				if (retry) {
> -					ClearPageHWPoison(p);
> +					clear_non_hugetlb_hwpoison(p);
>  					retry = false;
>  					goto try_again;
>  				}

[Severity: High]
Can memory_failure() miss this retry altogether when a page poisoned in
get_huge_page_for_hwpoison() becomes part of a new hugetlb folio?

In only_alloc_fresh_hugetlb_folio(), alloc_buddy_frozen_folio() and
alloc_gigantic_frozen_folio() remove the pages from the buddy allocator and
clear PageBuddy before init_new_hugetlb_folio() acquires hugetlb_lock and sets
PG_hugetlb.  When check_pages_enabled is false (the default when
CONFIG_DEBUG_VM is disabled and init_on_alloc/page_poison are off) or in
alloc_contig_frozen_range_noprof() (which ignores check_new_pages()'s return
value), the allocator does not reject a page with PG_hwpoison set:

CPU0 (memory_failure)             CPU1 (hugetlb allocation)
                                  only_alloc_fresh_hugetlb_folio()
                                    alloc_buddy_frozen_folio()
                                      /* clears PageBuddy(p) */
get_huge_page_for_hwpoison()
  /* !folio_test_hugetlb(folio) */
  TestSetPageHWPoison(p)
  return MF_HUGETLB_NON_HUGEPAGE
                                    init_new_hugetlb_folio()
                                      __folio_set_hugetlb(folio)
                                  hugetlb_vmemmap_optimize_folio()
                                    /* remaps p to shared tail page,
                                       frees old vmemmap page */
get_hwpoison_page(p, flags)
  get_hwpoison_hugetlb_folio()
    /* returns -EBUSY or 0 */
/* skips is_free_buddy_page(p) retry,
   returns MF_IGNORED */          <-- PG_hwpoison lost

Because the retry block in memory_failure() is only reached when
get_hwpoison_page() returns 0 and is_free_buddy_page(p) is true, a concurrent
hugetlb allocation that has already cleared PageBuddy bypasses the retry in
every hugetlb state:

1. While the new folio is still being initialized or HVO-optimized (neither
   HPG_freed nor HPG_migratable is set), get_hwpoison_hugetlb_folio() returns
   -EBUSY, get_any_page() returns -EIO after exhausting its retries, and
   memory_failure() exits via the default case (MF_MSG_GET_HWPOISON,
   MF_IGNORED).
2. Once the folio is placed on the hugetlb freelist (HPG_freed set),
   get_hwpoison_page() returns 0, is_free_buddy_page(p) is false, and
   memory_failure() exits via the MF_MSG_KERNEL_HIGH_ORDER (MF_IGNORED)
   branch.
3. If the folio is allocated for use and marked HPG_migratable,
   get_hwpoison_page() returns 1 and memory_failure() falls through into the
   non-hugetlb in-use page recovery path (hwpoison_filter(),
   split_huge_page_to_order(), etc.) on a hugetlb folio.

In (1) and (2), try_memory_failure_hugetlb() is never re-run: if HVO remaps a
tail page, the PG_hwpoison bit set in the old vmemmap page is discarded when
that vmemmap page is freed, leaving the corrupted physical page in the
hugetlb pool as a healthy page.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-hwpoison-hugetlb-ro-v3-1-c11186963647@columbia.edu?part=1