Re: [PATCH v4 5/5] mm/memory-failure: keep inherited poisoned frames out of the buddy allocator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Healthy pages within a memory block containing a poisoned page are completely leaked, preventing memory offlining.
- [Medium] Concurrent non-atomic updates to per-node memory failure statistics (mf_stats) cause lost updates and corrupted counters.
- [Medium] Memory block poison accounting is skipped during memory hotplug, breaking block-level poison tracking and causing global counter leaks.
- [Medium] Boot-poisoned pages lack the MAGIC_HWPOISON marker, breaking unpoison_memory() logic and causing a memory leak on unpoison.
- [Medium] TOCTOU race when setting the HWPoison flag during memory hotplug, potentially causing double accounting of memory failure stats.
--

commit 804f678a0cc93ac88d9e22aff45a011384c3ac87
Author: Breno Leitao <leitao@debian.org>

mm/memory-failure: keep inherited poisoned frames out of the buddy allocator

This commit checks if pages being given to the allocator are poisoned and marks
them accordingly. It hooks into __free_pages_core() so poisoned frames never
enter the buddy allocator. A new function hwpoison_boot_page() is used to set
the appropriate state for these pages.

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index d9b8be696aac3..f6afdb2a89a94 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c

[ ... ]

> +static void update_per_node_mf_stats(unsigned long pfn, enum mf_result result);
> +
> +/* Not num_poisoned_pages_inc(): its per block half divides by zero this early. */
> +void __meminit hwpoison_boot_page(struct page *page)
> +{
> +	if (PageHWPoison(page))
> +		return;
> +
> +	SetPageHWPoison(page);

[Severity: Medium]
Could a concurrent hardware MCE race with hwpoison_boot_page() here? During
memory hotplug, this path checks PageHWPoison(page) and then non-atomically
calls SetPageHWPoison(page). This might lead to both paths setting the flag
and double accounting in both global and per-node memory failure statistics.

> +	set_page_count(page, 1);
> +	/* The page has been completely isolated == MF_RECOVERED */

[Severity: Medium]
Does this code need to call SetPageHWPoisonTakenOff(page) here? Without the
MAGIC_HWPOISON marker, if unpoison_memory() is called on a boot-poisoned
page, it might not properly recognize the page as having been taken off the
buddy allocator, potentially causing a permanent memory leak on unpoison.

> +	update_per_node_mf_stats(page_to_pfn(page), MF_RECOVERED);

[Severity: Medium]
Are the non-atomic increments inside update_per_node_mf_stats() safe when
called concurrently during boot? Since __free_pages_core() can be executed
concurrently by multiple kthreads, the unprotected non-atomic increments like
++mf_stats->recovered might lead to corrupted per-node memory failure
statistics.

> +	atomic_long_inc(&num_poisoned_pages);

[Severity: Medium]
Does bypassing num_poisoned_pages_inc() here break memory hot-remove
accounting? By calling atomic_long_inc(&num_poisoned_pages) instead of
num_poisoned_pages_inc(), the hotplug memory block's nr_hwpoison counter is
left at 0. If the block is later hot-removed, the global num_poisoned_pages
counter would not be decremented properly.

> +}
> +
>  /**
>   * MF_ATTR_RO - Create sysfs entry for each memory failure statistics.

[ ... ]

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 404896b53003e..9e2ce833fd409 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c

[ ... ]

> @@ -1613,6 +1626,18 @@ void __meminit __free_pages_core(struct page *page, unsigned int order,
>  		atomic_long_add(nr_pages, &page_zone(page)->managed_pages);
>  	}
>  
> +	/* First: a block parked by __free_unaccepted() never returns here. */
> +	if (range_contains_poisoned_memory(page_to_phys(page),
> +					   PAGE_SIZE << order)) {
> +		poison_block(page, order);
> +		/*
> +		 * TODO: free the frames in the block that are not poisoned.
> +		 * They stay out of the allocator and still count in
> +		 * managed_pages, so a unit costs up to a block.
> +		 */
> +		return;
> +	}

[Severity: High]
Does this early return completely leak the unpoisoned pages within the memory
block? By returning early and bypassing __free_pages_ok(), the healthy pages
in the block are not returned to the buddy allocator. Since these leaked
pages lack PageBuddy, PageOffline, or PageHWPoison, memory offlining
(test_pages_isolated) might permanently fail with -EBUSY for this block.

> +
>  	if (page_contains_unaccepted(page, order)) {
>  		if (order == MAX_PAGE_ORDER && __free_unaccepted(page))
>  			return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-hwpoison-kho-v4-0-359313564495@debian.org?part=5