Re: [PATCH v5 05/12] hugetlb: Use the has_hwpoisoned flag

[Matthew Wilcox <willy@infradead.org>]

Sashiko:
> --- Patch [5]: [PATCH v5 05/12] hugetlb: Use the has_hwpoisoned flag ---
> commit cca87d250a7a597186556cb9d897807edae0b82f
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> hugetlb: Use the has_hwpoisoned flag
> 
> This commit converts hugetlb to use the has_hwpoisoned flag to match
> other large folios. It introduces folio_test_huge_poison() for lockless
> checks to close a gap where a poisoned hugetlb folio could be observed
> without the hwpoison bit set during teardown.
> 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> [ ... ]
> > @@ -1041,8 +1046,29 @@ PAGE_TYPE_OPS(Slab, slab, slab)
> >  
> >  #ifdef CONFIG_HUGETLB_PAGE
> >  FOLIO_TYPE_OPS(hugetlb, hugetlb)
> > +
> > +#ifdef CONFIG_MEMORY_FAILURE
> > +static inline bool folio_test_huge_poison(const struct folio *folio)
> > +{
> > +	return (READ_ONCE(folio->page.page_type) >> 23) ==
> > +		((PGTY_hugetlb << 1) | 1);
> > +}
> 
> This is a pre-existing issue, but does this logic fully close the race
> window?
> 
> The folio_test_huge_poison() function assumes the PGTY_hugetlb bits
> remain intact during teardown. However, __ClearPagehugetlb()
> unconditionally sets the entire page_type to UINT_MAX.
> 
> When __ClearPagehugetlb() is called, folio_test_huge_poison() will evaluate
> to false at the exact same moment folio_test_hugetlb() evaluates to false.
> Would this allow a concurrent is_page_hwpoison() check to miss the poison
> state?

Um, no?  Here's how is_page_hwpoison() looks:

static inline bool is_page_hwpoison(const struct page *page)
{
        const struct folio *folio;

        if (PageHWPoison(page))
                return true;
        folio = page_folio(page);
        if (folio_test_huge_poison(folio))
                return true;
        /* In case we raced with hugetlb transferring flags */
        return PageHWPoison(page);
}

We're relying on ordering; if we see a false folio_test_huge_poison(),
we recheck that the flag hasn't already been moved to the page.  Since
we always move the poison to the pages before clearing the page type,
I don't see how we can miss anything.

> > +static inline void folio_set_huge_poison(struct folio *folio)
> > +{
> > +	folio->page.page_type |= (1 << 23);
> > +}
> > +
> > +static inline void folio_clear_huge_poison(struct folio *folio)
> > +{
> > +	folio->page.page_type &= ~(1 << 23);
> > +}
> 
> Can this cause a data race on the page_type field?
> 
> In folio_clear_huge_poison(), the bitwise operation modifies the page_type
> field non-atomically. If unpoison_memory() calls this on a free hugetlb
> folio while a concurrent sysfs pool shrink calls __ClearPagehugetlb()
> under the hugetlb_lock, the UINT_MAX value could be overwritten with
> 0xFF7FFFFF.
> 
> Since unpoison_memory() relies on mf_mutex rather than hugetlb_lock,
> would this corrupted page_type trigger a VM_BUG_ON_PAGE in the buddy
> allocator when the page reaches free_pages_prepare()?

Hmm ... if we just move the hugetlb_clear_poison() call before the call
to folio_put(), that'll take care of this race, right?  Oh, but there's
two calls to folio_free_raw_hwp(), and the other one is when (ugh), we
don't have a reference to the folio.  So I think we wrap the whole thing
in hugetlb_lock to prevent this?

        ghp = get_hwpoison_page(p, MF_UNPOISON);
        if (!ghp) {
                spin_lock_irq(&hugetlb_lock);
                if (folio_test_hugetlb(folio)) {
                        huge = true;
                        count = folio_free_raw_hwp(folio, false);
                        if (count == 0)
                                goto unlock_mutex;
                        ret = hugetlb_clear_poison(folio);
                } else {
                        ret = TestClearPageHWPoison(p) ? 0 : -EBUSY;
                }
                spin_unlock_irq(&hugetlb_lock);

(this also includes the fix to the problem discussed below)

> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> [ ... ]
> > @@ -1498,10 +1498,10 @@ static void __update_and_free_hugetlb_folio(struct hstate *h,
> >  	}
> >  
> >  	/*
> > -	 * Move PageHWPoison flag from head page to the raw error pages,
> > -	 * which makes any healthy subpages reusable.
> > +	 * Move HWPoison flag to each error page
> > +	 * which makes any healthy pages reusable.
> >  	 */
> > -	if (unlikely(folio_test_hwpoison(folio)))
> > +	if (unlikely(folio_test_has_hwpoisoned(folio)))
> >  		folio_clear_hugetlb_hwpoison(folio);
> >  
> >  	VM_BUG_ON_FOLIO(folio_ref_count(folio), folio);
> 
> This is a pre-existing issue, but is there a risk of a double free in the
> hwpoison list teardown?
> 
> Both __update_and_free_hugetlb_folio() (via folio_clear_hugetlb_hwpoison())
> and unpoison_memory() can invoke __folio_free_raw_hwp() on the same folio
> concurrently. 
> 
> Looking at __folio_free_raw_hwp() in mm/memory-failure.c, it reads
> folio->hugetlb_hwpoison and sets it to NULL without any synchronization
> locks. If both threads read the pointer before either sets it to NULL,
> wouldn't they concurrently iterate the linked list and call kfree() on the
> same hwp_page elements?

I think that's right, but I also think it goes away later in this series
when hwp_page_lock serialises the calls to __folio_free_raw_hwp().

> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> [ ... ]
> > @@ -2730,8 +2753,8 @@ int unpoison_memory(unsigned long pfn)
> >  			count = folio_free_raw_hwp(folio, false);
> >  			if (count == 0)
> >  				goto unlock_mutex;
> > +			ret = hugetlb_clear_poison(folio);
> >  		}
> > -		ret = folio_test_clear_hwpoison(folio) ? 0 : -EBUSY;
> >  	} else if (ghp < 0) {
> >  		if (ghp == -EHWPOISON) {
> >  			ret = put_page_back_buddy(p) ? 0 : -EBUSY;
> > @@ -2740,17 +2763,18 @@ int unpoison_memory(unsigned long pfn)
> >  			unpoison_pr_info("%#lx: failed to grab page\n",
> >  					 pfn, &unpoison_rs);
> >  		}
> 
> Does this logic change drop the ability to unpoison free non-hugetlb pages?
> 
> The refactoring of the if (!ghp) block removed the fallback logic that
> handled non-hugetlb pages. If get_hwpoison_page() returns 0 for a free
> buddy page, execution enters the if (!ghp) block. 
> 
> Because folio_test_hugetlb(folio) is false, it does nothing, drops through
> the entire if/else chain, and returns -EBUSY. Could this leave free buddy
> pages permanently poisoned?

Oh, I see.  I hadn't realised that case was also being handled here.
I'll rework it.