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

[Matthew Wilcox <willy@infradead.org>]

Sashiko:
> --- Patch [1]: [PATCH v5 01/12] memory-failure: Fix hardware poison check in unpoison_memory() again ---
> commit 4beed8e27f2afa68261389ac2133cfe324d37d0e
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> memory-failure: Fix hardware poison check in unpoison_memory() again
> 
> This commit modifies unpoison_memory() to use is_page_hwpoison() to better
> support hugetlb folios and ensures the HWPoison flag is cleared on the correct
> head page.
> 
> > 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",
> 
> Can this speculative evaluation of is_page_hwpoison() trigger a panic?
> 
> Since this is evaluated without a reference on the page, the underlying
> page could be concurrently freed and reallocated as a tail page.
> Looking at is_page_hwpoison() in include/linux/page-flags.h, it calls
> folio_test_hugetlb(folio), which uses const_folio_flags(). That function

No it doesn't.  folio_test_hugetlb() is one of th FOLIO_TYPE_OPS which is
defined as return data_race(folio->page.page_type >> 24) == PGTY_##lname;
and no asserts.  I mean, there probably should be!  But this is irrelevant;
this whole function contains many such folio tests and the same problem
applies to all of them.  This patch doesn't make it worse.

> enforces a strict assertion:
> 
> VM_BUG_ON_PGFLAGS(PageTail(&folio->page))
> 
> If the page is concurrently reallocated as a tail page while
> is_page_hwpoison() executes, won't this crash the kernel on debug builds?
> 
> >  				 pfn, &unpoison_rs);
> >  		goto unlock_mutex;
> >  	}
> 
> [ ... ]
> 
> > @@ -2748,6 +2748,7 @@ int unpoison_memory(unsigned long pfn)
> >  				folio_put(folio);
> >  				goto unlock_mutex;
> >  			}
> > +			p = &folio->page;
> >  		}
> >  
> >  		folio_put(folio);
> 
> This isn't a bug introduced by this patch, but does this lead to a
> double folio_put() and use-after-free for hugetlb folios?
> 
> If memory_failure() handles an in-use hugetlb folio, me_huge_page()
> might fail to dissolve it, meaning no extra poison reference is taken
> but the folio remains marked as hwpoisoned. If the folio is later
> unmapped and holds a single transient reference, unpoison_memory()
> will acquire a reference (ghp = 1) and perform the folio_put(folio)
> seen just below this reassignment.

I'm not an expert in the memory-poison field, but this also seems wrong.
Don't all hugetlb folios have a folio_mapping()?  And thus we'll never
take the dissolve path in me_huge_page().

At any rate, it says "not introduced by this patch", so I'm not going
to take any action.

> By reassigning p = &folio->page here, the subsequent call to
> TestClearPageHWPoison(p) will be tested on the head page and return
> true, triggering a second unconditional folio_put(folio). Does this
> underflow the transient pin and cause a use-after-free?
> 
> 
> This is a pre-existing issue, but does this same logic underflow the
> refcount for THP folios that previously failed to split?
> 
> If memory_failure() handles a THP and try_to_split_thp_page() fails, it
> explicitly drops the reference acquired via get_hwpoison_page(p), leaving
> the THP with no extra poison pin. However, TestSetPageHWPoison(p) was
> already called.
> 
> If the THP later reaches a state with a single transient reference
> and no mappings, unpoison_memory() acquires a reference (ghp = 1),
> calls folio_put(folio) once, and then TestClearPageHWPoison(p) clears
> the flag and returns true. This triggers the second folio_put(folio),
> dropping a non-existent poison pin.

I don't know, and I'm losing patience with the Sashiko rabbit holes.
This is all getting very far afield from the problem I actually want
to fix.