Re: [PATCH v4 07/14] hugetlb: Use the has_hwpoisoned flag

[Matthew Wilcox <willy@infradead.org>]

Sashiko review:
> --- Patch [7]: [PATCH v4 07/14] hugetlb: Use the has_hwpoisoned flag ---
> commit f67cbd0b3ba7a67258490d423b9fc8352cb972aa
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> hugetlb: Use the has_hwpoisoned flag
> 
> Other large folios use the has_hwpoisoned flag.  Convert hugetlb to match.
> This will help us use the per-page hwpoison flag in the future.
> 
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 18691c905ac94..4200deab2c3c4 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> 
> [ ... ]
> 
> > @@ -1874,6 +1874,23 @@ static unsigned long __folio_free_raw_hwp(struct folio *folio, bool move_flag)
> >  #define	MF_HUGETLB_FOLIO_PRE_POISONED	3	/* folio already poisoned */
> >  #define	MF_HUGETLB_PAGE_PRE_POISONED	4	/* exact page already poisoned */
> >  #define	MF_HUGETLB_RETRY		5	/* hugepage is busy, retry */
> > +
> > +static inline int hugetlb_set_poison(struct folio *folio)
> > +{
> > +	if (folio_test_set_has_hwpoisoned(folio))
> > +		return MF_HUGETLB_FOLIO_PRE_POISONED;
> > +	folio_set_huge_poison(folio);
> > +	return 0;
> > +}
> 
> By replacing folio_test_set_hwpoison(folio) and no longer propagating 
> PG_hwpoison to the head page, does this break memory failure unmapping and 
> silently bypass hwpoison swap entry creation?
> 
> If a hardware memory error occurs on a tail page of a hugetlb folio, 
> memory_failure() calls TestSetPageHWPoison(p) on the exact tail page. It then 
> calls hugetlb_set_poison(), which sets PG_has_hwpoisoned but no longer sets 
> PG_hwpoison on the head page.
> 
> When unmapping the folio in try_to_unmap_one() in mm/rmap.c, the code 
> extracts the base PFN from the hugetlb PTE, calculates subpage as the head 
> page, and checks PageHWPoison(subpage):
> 
> mm/rmap.c:try_to_unmap_one() {
>     ...
>     VM_BUG_ON_PAGE(!PageHWPoison(subpage), subpage);
>     ...
>     if (PageHWPoison(subpage) && (flags & TTU_HWPOISON)) {
>         pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
>     ...
> }
> 
> Since PG_hwpoison is no longer on the head page, this check fails, triggering 
> VM_BUG_ON_PAGE. Without DEBUG_VM, the code skips inserting the hwpoison swap 
> entry, simply clearing the PTE, which silently allocates a new zeroed page on 
> subsequent accesses instead of delivering SIGBUS.

Oh; this is a good catch.  I have now reworked try_to_unmap() to handle
hugetlb pages a bit better and it'll be part of this patch in v5.

> > +
> > +static inline int hugetlb_clear_poison(struct folio *folio)
> > +{
> > +	if (!folio_test_clear_has_hwpoisoned(folio))
> > +		return -EBUSY;
> > +	folio_clear_huge_poison(folio);
> > +	return 0;
> > +}
> 
> Does this sequence open a race window with concurrent hugetlb freeing that 
> results in memory corruption?
> 
> When unpoison_memory() is called on a free hugetlb folio, 
> hugetlb_clear_poison() atomically clears PG_has_hwpoisoned first. This 
> exposes the folio to concurrent hugetlb demotion (demote_pool_huge_page()), 
> which can pick up the folio and call __update_and_free_hugetlb_folio(). This 
> function sets folio->page.page_type = UINT_MAX to return it to the buddy 
> allocator.
> 
> Concurrently, hugetlb_clear_poison() proceeds to execute 
> folio_clear_huge_poison() (defined in include/linux/page-flags.h), which 
> performs a non-atomic read-modify-write:
> 
> include/linux/page-flags.h:folio_clear_huge_poison() {
>     folio->page.page_type &= ~(1 << 23);
> }
> 
> This non-atomic write can overwrite the buddy allocator's UINT_MAX with 
> 0xFF7FFFFF. When the buddy allocator receives the page, it expects 
> page_type == UINT_MAX and will crash.

I'm not sure that it can be demoted while we're holding a reference to
the folio.  But I think it makes sense to change thusly:

+static inline int hugetlb_clear_poison(struct folio *folio)
+{
+	if (!folio_test_has_hwpoisoned(folio))
+		return -EBUSY;
+	folio_clear_huge_poison(folio);
+	folio_clear_has_hwpoisoned(folio));
+	return 0;
+}

(looking at Jiaqi's patches, maybe we shouldn't be clearing has_hwpoisoned
before freeing the folio?)