Re: [PATCH v2] 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 2 potential issue(s) to consider:
- [High] mm/memory-failure: missed retry when poisoned page becomes hugetlb folio
- [High] mm/memory-failure: broken HVO assumptions in clear_non_hugetlb_hwpoison()
--

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

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

This commit sets the hugetlb flag on newly initialized hugetlb folios and
sets PG_hwpoison on non-hugetlb pages while holding hugetlb_lock in
get_huge_page_for_hwpoison(). The goal is to prevent memory_failure() from
setting PG_hwpoison on the shared vmemmap tail page if the target page
concurrently becomes part of an HVO-optimized hugetlb folio.

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index b215312f261b8..80bac8d544d40 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c

[ ... ]

> @@ -2169,10 +2177,32 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags)
>  	return identify_page_state(pfn, p, page_flags);
>  }
>  
> +/*
> + * Keep the flag if the page has become a tail page of a hugetlb folio in
> + * the meantime, as HVO may remap its struct page.  A head page is never
> + * remapped, and keeping the flag there would make the retry take the
> + * folio for already poisoned.
> + */
> +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 == page)
> +		ClearPageHWPoison(page);
> +	spin_unlock_irq(&hugetlb_lock);
> +}

[Severity: High]
As reported by Sashiko on v1 [1], does clear_non_hugetlb_hwpoison() rely on
accurate assumptions about how HVO remaps struct page entries?

First, the comment states that a head page is never remapped, yet
__hugetlb_vmemmap_optimize_folio() allocates a new vmemmap_head page, copies
the first vmemmap page into it, and remaps &folio->page to vmemmap_head
outside hugetlb_lock:

mm/hugetlb_vmemmap.c:__hugetlb_vmemmap_optimize_folio() {
	vmemmap_head = alloc_pages_node(nid, GFP_KERNEL, 0);
	if (!vmemmap_head) {
		ret = -ENOMEM;
		goto out;
	}

	copy_page(page_to_virt(vmemmap_head), folio);
	...
	ret = vmemmap_remap_free(vmemmap_start, vmemmap_end,
				 vmemmap_head, vmemmap_tail,
				 vmemmap_pages, flags);
}

When page == &folio->page, if copy_page() runs after
get_huge_page_for_hwpoison() sets PG_hwpoison on page, and
clear_non_hugetlb_hwpoison() runs ClearPageHWPoison(page) before the PTE for
&folio->page is updated to vmemmap_head and TLB-flushed, ClearPageHWPoison()
only clears the flag on the old vmemmap page being freed. vmemmap_head keeps
PG_hwpoison set, so the retry in hugetlb_update_hwpoison() sees
folio_test_set_hwpoison(folio) == true and aborts with
MF_HUGETLB_FOLIO_PRE_POISONED (-EHWPOISON), leaving the poisoned hugetlb
folio undissolved.

Second, when page is a tail page (&folio->page != page) and
folio_test_hugetlb(folio) is true, clear_non_hugetlb_hwpoison()
unconditionally skips ClearPageHWPoison(page). However, tail pages 1..63
reside in the first 4KB vmemmap page (HUGETLB_VMEMMAP_RESERVE_SIZE), which is
copied to vmemmap_head and is never remapped to vmemmap_tail or overwritten
by __hugetlb_vmemmap_restore_folio(); and when HVO is disabled or fails, no
tail pages are remapped at all.

If PG_hwpoison is left set on one of those non-remapped tail struct pages,
can this cause recovery to abort with -EEXIST if the hugetlb folio is freed
before try_again re-acquires hugetlb_lock (such as when
alloc_surplus_hugetlb_folio() fails its overcommit recheck), or trigger
bad_page() upon buddy free if the hugetlb error is later filtered by
hwpoison_filter() or cleared via unpoison_memory()?

[ ... ]

> @@ -2486,7 +2517,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]
As reported by Sashiko on v1 [1], is this retry block actually reached when a
non-hugetlb page becomes part of a new hugetlb folio after
get_huge_page_for_hwpoison() sets PG_hwpoison and drops hugetlb_lock?

In only_alloc_fresh_hugetlb_folio(), alloc_buddy_frozen_folio() and
alloc_gigantic_frozen_folio() remove the pages from the buddy or contig
allocator (clearing PageBuddy and preparing the compound page) before calling
init_new_hugetlb_folio() to take hugetlb_lock and set PG_hugetlb;
demote_free_hugetlb_folios() similarly prepares split compound folios before
calling init_new_hugetlb_folio(). Moreover, when check_pages_enabled is false
or in alloc_contig_frozen_range_noprof() (which ignores the return value of
check_new_pages()), the page allocator does not reject a free buddy page with
PG_hwpoison set, and get_hwpoison_page() sleeps in zone_pcp_disable() before
inspecting p.

In memory_failure(), clear_non_hugetlb_hwpoison(p) and goto try_again are
gated behind get_hwpoison_page(p, flags) returning 0 and
is_free_buddy_page(p) returning true:

	res = get_hwpoison_page(p, flags);
	switch (res) {
	case 0:
		if (is_free_buddy_page(p)) {
			if (take_page_off_buddy(p)) {
				...
			} else {
				/* We lost the race, try again */
				if (retry) {
					clear_non_hugetlb_hwpoison(p);
					retry = false;
					goto try_again;
				}
				res = MF_FAILED;
			}
			res = action_result(pfn, MF_MSG_BUDDY, res);
		} else {
			res = action_result(pfn, MF_MSG_KERNEL_HIGH_ORDER, MF_IGNORED);
		}
		goto unlock_mutex;

Whenever the concurrent hugetlb allocation has already removed p from the
buddy freelist before memory_failure() reaches is_free_buddy_page(p), won't
the retry path be bypassed in all three hugetlb states?

1. If the new hugetlb folio is on the hugetlb freelist (HPG_freed set),
   get_hwpoison_hugetlb_folio() and get_any_page() return 0,
   is_free_buddy_page(p) is false, and memory_failure() exits via the
   MF_MSG_KERNEL_HIGH_ORDER branch without retrying.

2. If the new hugetlb folio is still in init_new_hugetlb_folio() or
   hugetlb_vmemmap_optimize_folios() (!HPG_freed and !HPG_migratable),
   get_hwpoison_hugetlb_folio() returns -EBUSY, get_any_page() returns -EIO
   after exhausting its retries, and memory_failure() exits via the default
   MF_MSG_GET_HWPOISON branch without retrying.

3. If the new hugetlb folio has already become in-use (HPG_migratable set),
   get_hwpoison_hugetlb_folio() pins the folio and returns 1, and
   get_any_page() sees PageHuge(p) and returns 1. memory_failure() then falls
   through into the non-hugetlb in-use path with a hugetlb folio: if
   hwpoison_filter(p) matches, it calls ClearPageHWPoison(p) without
   hugetlb_lock (touching the shared vmemmap_tail page if HVO-optimized), and
   otherwise it calls folio_set_has_hwpoisoned(folio) and
   split_huge_page_to_order() on the hugetlb folio.

In the first two cases, if p became a remapped tail page (>= 64) of an
HVO-optimized hugetlb folio, freeing the original vmemmap page discards
PG_hwpoison and leaves the poisoned hugetlb folio in the pool; if p became
the head page, PG_hwpoison stays set with an empty raw_hwp_page list and is
cleared when the folio is later freed; and if p became a non-remapped tail
page, PG_hwpoison stays on the tail struct page and triggers bad_page() when
the folio is freed.

[1] https://lore.kernel.org/r/20261101-hwpoison-hugetlb-ro-v1-1-a42d6411cb65@columbia.edu

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-hwpoison-hugetlb-ro-v2-1-43876fc9eef1@columbia.edu?part=1