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

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.

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.