Re: [PATCH v7 01/13] memory-failure: Fix hardware poison check in unpoison_memory() again

On 7/31/2026 6:39 AM, Matthew Wilcox wrote:
> On Thu, Jul 30, 2026 at 10: 39: 57PM -0700, jane. chu@ oracle. com 
> wrote: > I think I spot another pre-existing issue in unpoison_memory(): 
>  > the XXX line doesn't work for non-hugetlb free large folio because 
> it'll > just try to clear PG_hwpoison
> 
> 
> On Thu, Jul 30, 2026 at 10:39:57PM -0700, [email protected] wrote:
>> I think I spot another pre-existing issue in unpoison_memory():
>> the XXX line doesn't work for non-hugetlb free large folio because it'll
>> just try to clear PG_hwpoison in the folio->page, not the precise 'pfn'
>> page.  Something like below could do.
>> 
>> 
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 51508a55c405..14915718ace5 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2730,8 +2730,10 @@ int unpoison_memory(unsigned long pfn)
>>                         count = folio_free_raw_hwp(folio, false);
>>                         if (count == 0)
>>                                 goto unlock_mutex;
>> +                       else
>> +                               ret = folio_test_clear_hwpoison(folio) ? 0 :
>> -EBUSY;
>>                 }
>> -               ret = folio_test_clear_hwpoison(folio) ? 0 : -EBUSY; <---
>> XXX
>> +               ret = TestClearPageHWPoison(p) ? 0 : -EBUSY;
>>         } else if (ghp < 0) {
>>                 if (ghp == -EHWPOISON) {
>>                         ret = put_page_back_buddy(p) ? 0 : -EBUSY;
> 
> I think you're right, but this isn't a bug.  That is, we're calling the
> wrong function, but in this particular branch, we're guaranteed that
> 'folio' and 'p' have the same value so the same bit is cleared.
> 
> This is the case where !ghp is true.  Here's the code we're looking at:
> 
>          ghp = get_hwpoison_page(p, MF_UNPOISON);
>          if (!ghp) {
>                  if (folio_test_hugetlb(folio)) {
>                          huge = true;
>                          count = folio_free_raw_hwp(folio, false);
>                          if (count == 0)
>                                  goto unlock_mutex;
>                  }
>                  ret = folio_test_clear_hwpoison(folio) ? 0 : -EBUSY;
> 
> It is my understanding that we take this path for hugetlb memory and
> pages which are in the buddy allocator.  Buddy pages aren't compound
> pages, so calling page_folio() on them returns the same pointer, just
> cast to a folio.

Agreed. I think the header of get_hwpoison_page() can take a bit revise -
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1484,7 +1484,7 @@ static int __get_unpoison_page(struct page *page)
   * the given page has PG_hwpoison. So it's never reused for other page
   * allocations, and __get_unpoison_page() never races with them.
   *
- * Return: 0 on failure or free buddy (hugetlb) page,
+ * Return: 0 on free buddy or hugetlb page,
   *         1 on success for in-use pages in a well-defined state,
   *         -EIO for pages on which we can not handle memory errors,
   *         -EBUSY when get_hwpoison_page() has raced with page lifecycle


> 
> Now, I do fix this in "hugetlb: Use the has_hwpoisoned flag" (because
> nobody gets to use folio_test_clear_hwpoison() any more):
> 
> @@ -2733,8 +2756,10 @@ int unpoison_memory(unsigned long pfn)
>                                  hugetlb_unlock_irq();
>                                  goto unlock_mutex;
>                          }
> +                       ret = hugetlb_clear_poison(folio);
> +               } else {
> +                       ret = TestClearPageHWPoison(p) ? 0 : -EBUSY;
>                  }
> -               ret = folio_test_clear_hwpoison(folio) ? 0 : -EBUSY;
>                  hugetlb_unlock_irq();
>          } else if (ghp < 0) {
>                  if (ghp == -EHWPOISON) {
> 
> so I agree with you this should be fixed.  But I don't think it fixes
> an actual bug.  If it did, then we should do that first for easier
> backporting.  So if I've got anything wrong here, it's worth saying.
> 

Yes, with your [PATCH v7 06/13], the patch looks fine.

Reviewed-by: Jane Chu <[email protected]>

thanks,
-jane