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

[Miaohe Lin <linmiaohe@huawei.com>]

On 2026/11/7 0:09, Mani Tofigh via B4 Relay wrote:
> From: Mani Tofigh <mt4022@columbia.edu>
> 

Thanks for your patch. Some questions below.

> 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

I have no idea why the error on P itself is ignored, could you explain that?

> 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 part of a hugetlb folio in
> the meantime and its struct page isn't in the first vmemmap page of the
> folio, which HVO doesn't share.  The retry then knows the flag is its
> own.  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 v3:
> - Keep the flag only for a page beyond the first vmemmap page of the
>   folio, and fix the comment that said a head page is never remapped
>   (Sashiko)
> - Don't take a flag kept by the retry path for an earlier error (Sashiko)
> - Rebase onto current mm-new
> - Link to v2: https://patch.msgid.link/20261103-hwpoison-hugetlb-ro-v2-1-43876fc9eef1@columbia.edu
> 
> Reproducer: https://github.com/manitofigh/files/blob/main/linux-repros/hugetlbro.c
> ---
>  mm/hugetlb.c        |  3 +++
>  mm/memory-failure.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 4eb7a8c1f7f6..da4ae43de9f7 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 78f0a13349cb..9795f07b26f5 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -61,6 +61,7 @@
>  #include <linux/pagewalk.h>
>  #include <linux/shmem_fs.h>
>  #include <linux/sysctl.h>
> +#include <linux/vmemmap-optimization.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/memory-failure.h>
> @@ -1957,6 +1958,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.
> @@ -2041,7 +2043,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;
> @@ -2092,7 +2098,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
> @@ -2111,6 +2118,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;
> @@ -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)

I think we should clear the PageHWPoison flag we just set since the retry path expects
we have cleared the PageHWPoison flag in the first round.

> +		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)
> @@ -2451,12 +2482,14 @@ 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)) {
> +	/* On the retry, the flag may still be set from the first try. */
> +	if (res == -EEXIST && retry) {
>  		res = -EHWPOISON;
>  		if (flags & MF_ACTION_REQUIRED)
>  			res = kill_accessing_process(current, pfn, flags);
> @@ -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);

I suspect this won’t solve the problem. The below race might still exist:

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

Or am I miss something?

Thanks.
.