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

[Matthew Wilcox <willy@infradead.org>]

Sashiko feedback:
> --- Patch [1]: [PATCH v4 01/14] memory-failure: Fix hardware poison check in unpoison_memory() again ---
> commit 5a43cce770d0c716bb5c78ee2c21e1cb711e5331
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> memory-failure: Fix hardware poison check in unpoison_memory() again
> 
> This commit addresses a bug in unpoison_memory() for hugetlb folios. It uses
> is_page_hwpoison() instead of checking the head page, and correctly targets
> the precise page for clearing the HWPoison flag.
> 
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 51508a55c4055..944e6e1d49719 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -2695,8 +2695,8 @@ int unpoison_memory(unsigned long pfn)
> >  		goto unlock_mutex;
> >  	}
> >  
> > -	if (!PageHWPoison(p)) {
> > -		unpoison_pr_info("%#lx: page was already unpoisoned\n",
> > +	if (!is_page_hwpoison(p)) {
> > +		unpoison_pr_info("%#lx: page is not poisoned\n",
> >  				 pfn, &unpoison_rs);
> >  		goto unlock_mutex;
> >  	}
> 
> This isn't a bug introduced by this patch, but can the speculative folio
> access without stabilization cause a use-after-free and reference leak?
> 
> Looking at unpoison_memory(), if the PFN belongs to a THP tail page and
> the THP is concurrently split, could the folio pointer refer to a stale
> head page?
> 
> unpoison_memory() {
>         ...
>         folio = page_folio(p);
>         ...
>         ghp = get_hwpoison_page(p, MF_UNPOISON);
>         ...
>         folio_put(folio);
> }
> 
> Since get_hwpoison_page() takes a reference on the new order-0 page, does
> calling folio_put() on the stale head page drop the wrong reference and leak
> the reference on the order-0 page?

Uh.  I don't know.  If it does, I didn't make this problem _worse_,
so I'm not going to address it.

> This is a pre-existing issue, but does unpoison_memory() clear the
> PG_hwpoison flag on the wrong page for unsplit THPs?
> 
> For THP tail pages, if get_hwpoison_page() returns 0 because their
> _refcount is 0, does the code fall into the !ghp branch and call
> folio_test_clear_hwpoison(folio)?
> 
> Because PF_HEAD redirects to the head page, this clears the flag on the
> head page. Since THPs set PG_hwpoison on the exact corrupted tail page
> (PF_ANY), does the tail page remain poisoned and fail to restore?

Hm.  Maybe?  This code probably needs to be more careful in general.

> [ ... ]
> 
> > @@ -2748,6 +2748,7 @@ int unpoison_memory(unsigned long pfn)
> >  				folio_put(folio);
> >  				goto unlock_mutex;
> >  			}
> > +			p = &folio->page;
> >  		}
> >  
> >  		folio_put(folio);
> 
> This is a pre-existing issue, but does unpoison_memory() fail to clear
> the per-folio PG_has_hwpoisoned flag when unpoisoning THPs?
> 
> When successfully unpoisoning a THP head page (where get_hwpoison_page
> succeeds), TestClearPageHWPoison() clears the per-page PG_hwpoison flag.
> 
> However, since the per-folio fast-indicator PG_has_hwpoisoned is never
> cleared, will folio_contain_hwpoisoned_page() continue to return true,
> causing the kernel to erroneously treat the unpoisoned THP as poisoned in
> future operations?

Yes, it does fail to clear the has_hwpoisoned flag.  We can have more
than one HWPoison page in a folio, so we either have to leave it set,
or scan all pages in the folio to know that it's safe to clear.

Again, I'm not making this worse for THP in this series, so I'm
not addressing it.  For hugetlb pages, later on I change to use the
has_hwpoisoned flag, and that does get cleared because there's no scan
of page bits involved, just a list of poisoned pages, and it's easy to
check when that's empty.

It's mostly moot because we always try to split large folios that aren't
THP, and that usually succeeds.  So all the large folio has_hwpoisoned
paths are very undertested.