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 <[email protected]> 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/[email protected]?part=5
