Re: [PATCH v2] mm/memory_hotplug: don't offline a hwpoisoned page still in the page cache

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] mm/memory_hotplug: scan_movable_pages() misses poisoned tail
  pages of folios
--

commit 3a170d5683c399929cdc4fe6d9523b3ecee4c1c1
Author: Mani Tofigh <mt4022@columbia.edu>

mm/memory_hotplug: don't offline a hwpoisoned page still in the page cache

This commit prevents memory offlining from skipping hwpoisoned pages whose
folios are still in the page cache or swap cache by introducing
hwpoison_page_offlineable(). It updates scan_movable_pages(),
do_migrate_range(), and __test_page_isolated_in_pageblock() so that
offline_pages() fails with -EBUSY instead of offlining memory still
referenced by a mapping or racing with memory_failure().

[ ... ]

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 1bfd1f4daeb20..c5cfaa24b41b7 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1821,6 +1821,8 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>  		struct folio *folio;
>  
>  		page = pfn_to_page(pfn);
> +		if (PageHWPoison(page) && !hwpoison_page_offlineable(page))
> +			return -EBUSY;
>  		if (PageLRU(page) || page_has_movable_ops(page))
>  			goto found;
>  
> @@ -1887,6 +1889,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  			 */
>  			if (folio_test_large(folio) && !folio_test_hugetlb(folio))
>  				goto put_folio;
> +			/* The offline fails on it anyway, leave it to memory_failure() */
> +			if (!hwpoison_page_offlineable(page))
> +				goto put_folio;
>  			if (folio_test_lru(folio) && !folio_isolate_lru(folio))
>  				goto put_folio;
>  			if (folio_mapped(folio)) {

[Severity: High]
As reported by Sashiko on v1 [1], what happens here when a tail page of a
large page-cache or swap-cache folio is hwpoisoned?

Because PG_hwpoison uses PF_ANY while PageLRU() uses PF_HEAD, a large LRU
folio with a poisoned tail page has PageHWPoison(page) false and
PageLRU(page) true on its head page. Since scan_movable_pages() scans PFNs in
ascending order, it encounters the head page first and takes the PageLRU(page)
branch:

    page = pfn_to_page(pfn);
    if (PageHWPoison(page) && !hwpoison_page_offlineable(page))
        return -EBUSY;
    if (PageLRU(page) || page_has_movable_ops(page))
        goto found;

returning 0 with *movable_pfn set to the head PFN without ever scanning the
poisoned tail PFN.

In offline_pages(), the inner loop then passes that head PFN by value to
do_migrate_range():

    do {
        ...
        ret = scan_movable_pages(pfn, end_pfn, &pfn);
        if (!ret) {
            do_migrate_range(pfn, end_pfn);
        }
    } while (!ret);

Inside do_migrate_range(), pfn is advanced past all tail pages of the large
folio:

    if (folio_test_large(folio))
        pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;

    if (folio_contain_hwpoisoned_page(folio)) {
        if (folio_test_large(folio) && !folio_test_hugetlb(folio))
            goto put_folio;
        if (!hwpoison_page_offlineable(page))
            goto put_folio;

This appears to leave two regressions for large folios:

First, if PG_has_hwpoisoned is already set on the large folio (and it was not
split, for instance because min_order_for_split() > 0 or split failed),
do_migrate_range() takes goto put_folio without isolating the folio from the
LRU. On the next iteration in offline_pages(), pfn is still the head PFN, so
scan_movable_pages() stops at the head page again and offline_pages() spins
in an infinite loop while holding mem_hotplug_lock and pcp_batch_high_lock.

Second, if memory_failure() on a tail page has called TestSetPageHWPoison(p)
and is blocked in get_hwpoison_page() waiting on zone_pcp_disable() before
reaching folio_set_has_hwpoisoned(folio), folio_contain_hwpoisoned_page(folio)
is still false. In that window, do_migrate_range() skips the tail PFNs and
isolates and migrates the folio instead of leaving it to memory_failure().

Should scan_movable_pages() and do_migrate_range() check for poisoned
subpages across the folio (or check folio_contain_hwpoisoned_page() in
scan_movable_pages()) so large folios with poisoned tail pages also return
-EBUSY?

[1] https://lore.kernel.org/r/20261101-memhp-hwpoison-pagecache-v1-1-ed8785ca44eb@columbia.edu

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-memhp-hwpoison-pagecache-v2-1-029d35634d42@columbia.edu?part=1