Re: [PATCH v4 09/14] mm: Check individual hugetlb pages for poison

[Matthew Wilcox <willy@infradead.org>]

Sashiko review:
> --- Patch [9]: [PATCH v4 09/14] mm: Check individual hugetlb pages for poison ---
> commit 5da9009498dbc7d3d1fea29d7065847731ac20dc
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> Subject: mm: Check individual hugetlb pages for poison
> 
> This commit modifies the hugetlb hardware poison checking logic to verify
> individual subpages instead of the entire folio. It introduces a new
> function to safely perform this check without holding a folio reference by
> relying on the hugetlb_lock.
> 
> Link: N/A
> 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 7c4652dd517a3..9cde74320e48a 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> 
> [ ... ]
> 
> > @@ -1090,6 +1090,8 @@ static inline bool PageHuge(const struct page *page)
> >  	return folio_test_hugetlb(page_folio(page));
> >  }
> >  
> > +bool hugetlb_unref_page_hwpoison(const struct page *page);
> 
> Does this cause a build failure when CONFIG_MEMORY_FAILURE is not set?
> 
> When CONFIG_MEMORY_FAILURE=n, mm/memory-failure.c isn't compiled, meaning
> hugetlb_unref_page_hwpoison() is not defined.
> 
> Since is_page_hwpoison() is an inline function that evaluates a runtime
> condition when CONFIG_HUGETLB_PAGE=y, the compiler can't optimize the
> branch away.
> 
> Will this result in an undefined reference to hugetlb_unref_page_hwpoison()
> for callers like fs/proc/kcore.c?

Oh, right, need to define FOLIO_TEST_FLAG_FALSE(huge_poison) if
CONFIG_MEMORY_FAILURE isn't defined.

> > @@ -1104,7 +1106,7 @@ static inline bool is_page_hwpoison(const struct page *page)
> >  		return true;
> >  	folio = page_folio(page);
> 
> This is a pre-existing issue, but can this speculative folio access cause a
> kernel panic?
> 
> In PFN-scanning contexts like /proc/kcore, is_page_hwpoison() is called
> without holding a stabilizing reference.
> 
> It calls page_folio() speculatively and reads flags via
> folio_test_huge_poison().
> 
> If the page is concurrently freed and reallocated to a non-compound user
> (like SLAB) that stores data in the compound_head field, page_folio() will
> return a garbage pointer.

... what?  Slab is a good compound page user.  You're drinking the
bad bot juice again.

> This isn't a bug introduced by this patch, but could the non-atomic
> modification of the poison flag corrupt the buddy allocator?
> 
> unpoison_memory() calls hugetlb_clear_poison(folio) which performs a
> non-atomic read-modify-write:
> 
>     folio->page.page_type &= ~(1 << 23);
> 
> When get_hwpoison_page() returns 0 for a free hugetlb folio,
> unpoison_memory() does not hold a reference. Concurrently, the folio can
> be freed to the buddy allocator.
> 
> If the non-atomic modification in unpoison_memory() races with
> __folio_clear_hugetlb() or the buddy allocator's __SetPageBuddy(), it can
> overwrite their updates.
> 
> For example, if it overwrites the buddy flag, PageBuddy() becomes
> false for a page on the buddy list, crashing the allocator when accessed.

I think it re-found the same problem it complained about earlier.

> > +bool hugetlb_unref_page_hwpoison(const struct page *page)
> > +{
> > +	const struct folio *folio;
> > +	unsigned long flags;
> > +	bool ret;
> > +
> > +	spin_lock_irqsave(&hugetlb_lock, flags);
> > +	folio = page_folio(page);
> > +	if (!folio_test_huge_poison(folio)) {
> > +		ret = PageHWPoison(page);
> > +		goto unlock;
> > +	}
> > +
> > +	ret = precise_page_poisoned(folio, page);
> 
> Can this list traversal result in a use-after-free?

As earlier, I'll protect this with a spinlock.