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

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

[Mani Tofigh via B4 Relay]

From: Mani Tofigh <mt4022@columbia.edu>

memory_failure() checks under hugetlb_lock that the page isn't part of
a hugetlb folio and sets PG_hwpoison only after dropping the lock.  The
page can become a tail page of a new hugetlb folio in the meantime.
With hugetlb_free_vmemmap=on its struct page then lives in the tail
page that all such folios in the zone share, so the flag lands on tail
pages of every one of them:

  Task A (memory_failure)              Task B (nr_hugepages write)
  get_huge_page_for_hwpoison()
    /* P not hugetlb */
                                       alloc_fresh_hugetlb_folio()
                                         /* P becomes a tail page */
                                         hugetlb_vmemmap_optimize_folio()
                                           /* P's struct page now in
                                              the shared tail page */
  TestSetPageHWPoison(P)

The error on P itself is ignored, while healthy pages of other folios
stay marked and are never reused once their folio is freed.  After
freeing the pool, /proc/kpageflags shows them as hwpoisoned.

This is the allocation side of the race that commit 32c877191e02
("hugetlb: do not clear hugetlb dtor until allocating vmemmap") fixed
for freeing.

Set the hugetlb flag of a new folio under hugetlb_lock and set
PG_hwpoison of a non-hugetlb page inside the locked check, so that the
vmemmap can't be optimized in between.  The retry path clears it under
the same lock, unless the page has become a tail page of a hugetlb folio
in the meantime.  With the fix, no healthy page gets marked anymore.

This bug was identified and confirmed with the help of a deterministic
hypervisor currently under development.  The C reproducer below however
hits it within about 35s.

Fixes: f41f2ed43ca5 ("mm: hugetlb: free the vmemmap pages associated with each HugeTLB page")
Cc: stable@vger.kernel.org
Signed-off-by: Mani Tofigh <mt4022@columbia.edu>
---
Changes in v2:
- In the retry path, also clear the flag if the page has become the
  head page of a hugetlb folio, so that the retry doesn't take the
  folio for already poisoned (Sashiko)
- Rebase on mm-new
- Link to v1: https://patch.msgid.link/20261101-hwpoison-hugetlb-ro-v1-1-a42d6411cb65@columbia.edu

Reproducer: https://github.com/manitofigh/files/blob/main/linux-repros/hugetlbro.c
---
 mm/hugetlb.c        |  3 +++
 mm/memory-failure.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 779b1de33c78..dd409d7f697e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1792,7 +1792,10 @@ static void account_new_hugetlb_folio(struct hstate *h, struct folio *folio)
 
 void init_new_hugetlb_folio(struct folio *folio)
 {
+	/* Pairs with get_huge_page_for_hwpoison(), see there. */
+	spin_lock_irq(&hugetlb_lock);
 	__folio_set_hugetlb(folio);
+	spin_unlock_irq(&hugetlb_lock);
 	INIT_LIST_HEAD(&folio->lru);
 	hugetlb_set_folio_subpool(folio, NULL);
 	set_hugetlb_cgroup(folio, NULL);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b215312f261b..80bac8d544d4 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1956,6 +1956,7 @@ static unsigned long __folio_free_raw_hwp(struct folio *folio, bool move_flag)
 #define	MF_HUGETLB_FOLIO_PRE_POISONED	3	/* folio already poisoned */
 #define	MF_HUGETLB_PAGE_PRE_POISONED	4	/* exact page already poisoned */
 #define	MF_HUGETLB_RETRY		5	/* hugepage is busy, retry */
+#define	MF_HUGETLB_NON_HUGEPAGE_POISONED	6	/* not a hugepage, already poisoned */
 /*
  * Set hugetlb folio as hwpoisoned, update folio private raw hwpoison list
  * to keep track of the poisoned pages.
@@ -2040,7 +2041,11 @@ static int get_huge_page_for_hwpoison(unsigned long pfn, int flags,
 	spin_lock_irq(&hugetlb_lock);
 	folio = page_folio(page);
 	if (!folio_test_hugetlb(folio)) {
-		ret = MF_HUGETLB_NON_HUGEPAGE;
+		/* Mark it before it can become part of a new hugetlb folio. */
+		if (TestSetPageHWPoison(page))
+			ret = MF_HUGETLB_NON_HUGEPAGE_POISONED;
+		else
+			ret = MF_HUGETLB_NON_HUGEPAGE;
 		goto out_unlock;
 	} else if (flags & MF_COUNT_INCREASED) {
 		ret = MF_HUGETLB_IN_USED;
@@ -2091,7 +2096,8 @@ static int get_huge_page_for_hwpoison(unsigned long pfn, int flags,
  * PageHWPoison) should be done in single hugetlb_lock range.
  * Returns:
  *	0		- recovered
- *	-ENOENT		- no hugetlb page
+ *	-ENOENT		- no hugetlb page, now marked hwpoison
+ *	-EEXIST		- no hugetlb page, already marked hwpoison
  *	-EBUSY		- not recovered
  *	-EOPNOTSUPP	- hwpoison_filter'ed
  *	-EHWPOISON	- folio or exact page already poisoned
@@ -2110,6 +2116,8 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags)
 	switch (res) {
 	case MF_HUGETLB_NON_HUGEPAGE:	/* fallback to normal page handling */
 		return -ENOENT;
+	case MF_HUGETLB_NON_HUGEPAGE_POISONED:
+		return -EEXIST;
 	case MF_HUGETLB_RETRY:
 		if (!(flags & MF_NO_RETRY)) {
 			flags |= MF_NO_RETRY;
@@ -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);
+}
+
 #else
 static inline int try_memory_failure_hugetlb(unsigned long pfn, int flags)
 {
-	return -ENOENT;
+	return TestSetPageHWPoison(pfn_to_page(pfn)) ? -EEXIST : -ENOENT;
+}
+
+static inline void clear_non_hugetlb_hwpoison(struct page *page)
+{
+	ClearPageHWPoison(page);
 }
 
 static inline unsigned long folio_free_raw_hwp(struct folio *folio, bool flag)
@@ -2450,12 +2480,13 @@ int memory_failure(unsigned long pfn, int flags)
 try_again:
 	res = try_memory_failure_hugetlb(pfn, flags);
 	/*
-	 * -ENOENT means the page we found is not hugetlb, so proceed with normal page handling
+	 * -ENOENT and -EEXIST mean the page is not hugetlb and is now marked
+	 * hwpoison, so proceed with normal page handling.
 	 */
-	if (res != -ENOENT)
+	if (res != -ENOENT && res != -EEXIST)
 		goto unlock_mutex;
 
-	if (TestSetPageHWPoison(p)) {
+	if (res == -EEXIST) {
 		res = -EHWPOISON;
 		if (flags & MF_ACTION_REQUIRED)
 			res = kill_accessing_process(current, pfn, flags);
@@ -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;
 				}

---
base-commit: 74d73ffcffe03804f97e9fb82301dadf75671282
change-id: 20261030-hwpoison-hugetlb-ro-5f8881a60af9

Best regards,
--  
Mani Tofigh <mt4022@columbia.edu>

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

[sashiko-bot]

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