Re: [RFC PATCH v2 09/18] mm/memory-failure: handle folios with PG_rcu_free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/memory-failure: unsafe RCU wait in memory_failure() already-poisoned path
- [High] mm/memory-failure: mf_free_rcu_pages() skips sync while rcu_free_pages() runs
- [Medium] mm/memory-failure: order-0 PG_rcu_free page dropped after setting PG_hwpoison
--

commit 3d8069abfad786882a48d6ce85e757674239a9ca
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

mm/memory-failure: handle folios with PG_rcu_free

This commit updates memory_failure() and get_any_page() to drain pages
waiting for an RCU grace period via rcu_free_sync() and waits for a
grace period via synchronize_rcu() before returning success on a
PG_rcu_free folio.

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 7dde7c3c9a0c5..398463b2e82d5 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1419,6 +1419,19 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags)
>  	return 0;
>  }
>  
> +/* A page waiting for a grace period is not in the buddy yet: free it */
> +static void mf_free_rcu_pages(struct page *p)
> +{
> +	if (!zone_rcu_free_waiting(page_zone(p)))
> +		return;
> +	if (page_count(p))
> +		return;
> +	if (is_free_buddy_page(p))
> +		return;
> +
> +	rcu_free_sync();
> +}

[Severity: High]
As reported by Sashiko on RFC [1], can mf_free_rcu_pages() return early
without calling rcu_free_sync() while rcu_free_pages() is still freeing
the target folio?

In mm/page_alloc.c:rcu_free_pages(), NR_FREE_RCU_PAGES is decremented
before the folios are actually freed to the buddy allocator:

mm/page_alloc.c:rcu_free_pages() {
	list_for_each_entry_safe(folio, next, list, lru) {
		nr += folio_nr_pages(folio);
		mod_zone_page_state(folio_zone(folio), NR_FREE_RCU_PAGES,
				    -folio_nr_pages(folio));
		if (direct) {
			__free_pages_ok(&folio->page, folio_order(folio),
					FPI_NONE);
			cond_resched();
		} else if (!folio_batch_add(&fbatch, folio)) {
			free_unref_folios(&fbatch);
			cond_resched_tasks_rcu_qs();
		}
	}
	if (folio_batch_count(&fbatch))
		free_unref_folios(&fbatch);
}

Similarly, during folio release, folio_put_testzero() drops _refcount to
0 before __free_pages_prepare() -> rcu_free_defer() increments
NR_FREE_RCU_PAGES.

When the last waiting folios in a zone are in either window,
page_count(p) is 0, is_free_buddy_page(p) is false, and
zone_rcu_free_waiting(page_zone(p)) evaluates to false.

If memory_failure() calls mf_free_rcu_pages(p) during this window, it
skips rcu_free_sync() and proceeds to TestSetPageHWPoison(p) before
__free_pages_prepare() runs. For an order-0 page, __free_pages_prepare()
then sees PageHWPoison(page) && !order and drops the page instead of
placing it in the buddy allocator, causing get_hwpoison_page() to fail
with -EIO and leaving the hardware error ignored as MF_IGNORED.

In addition, because memory_failure() calls mf_free_rcu_pages(p) before
get_hwpoison_page() calls zone_pcp_disable(), if rcu_free_workfn() or
rcu_free_expedite_workfn() already spliced p off the per-CPU list,
rcu_free_pages(..., false) -> free_unref_folios() will free p onto a
per-CPU pageset (PCP) list rather than the buddy allocator before
TestSetPageHWPoison(p) runs.

> +
>  #define GET_PAGE_MAX_RETRY_NUM 3
>  
>  static int get_any_page(struct page *p, unsigned long flags)
> @@ -1443,6 +1456,7 @@ static int get_any_page(struct page *p, unsigned long flags)
>  	}
>  
>  try_again:
> +	mf_free_rcu_pages(p);
>  	if (!count_increased) {

[Severity: Medium]
As reported by Sashiko on RFC [2], what happens in memory_failure() if an
in-use order-0 folio with PG_rcu_free is freed on another CPU after the
initial mf_free_rcu_pages(p) call?

In memory_failure(), the initial mf_free_rcu_pages(p) call returns
immediately when page_count(p) > 0:

mm/memory-failure.c:memory_failure() {
	mf_free_rcu_pages(p);
	if (TestSetPageHWPoison(p)) {
		...
	}
	res = get_hwpoison_page(p, flags);
}

If another CPU drops the last reference on that order-0 folio just before
TestSetPageHWPoison(p), __free_pages_prepare() queues the folio via
rcu_free_defer(folio), and TestSetPageHWPoison(p) then sets PG_hwpoison on
p while it is on the RCU free list. When get_hwpoison_page() ->
get_any_page() then calls mf_free_rcu_pages(p) at try_again,
rcu_free_sync() drains p through __free_pages_ok() ->
__free_pages_prepare():

mm/page_alloc.c:__free_pages_prepare() {
	if (unlikely(folio_test_rcu_free(folio)) &&
	    !(PageHWPoison(page) && !order)) {
		...
		rcu_free_defer(folio);
		return false;
	}
	...
	if (unlikely(PageHWPoison(page)) && !order) {
		/* Do not let hwpoison pages hit pcplists/buddy */
		...
		return false;
	}
}

Because PageHWPoison(p) is already set and order is 0,
__free_pages_prepare() drops the page instead of placing it in the buddy
allocator. And if the last reference is dropped after
TestSetPageHWPoison(p) while get_any_page() is retrying,
__free_pages_prepare() sees PageHWPoison(page) && !order, skips
rcu_free_defer(folio) without waiting for an RCU grace period, and also
drops the page.

In both cases, is_free_buddy_page(p) remains false, get_any_page() fails
with -EIO after retrying, and memory_failure() reports MF_IGNORED without
waiting for lockless RCU readers or accounting the page as recovered.

[ ... ]

> @@ -2440,6 +2454,7 @@ int memory_failure(unsigned long pfn, int flags)
>  	if (res != -ENOENT)
>  		goto unlock_mutex;
>  
> +	mf_free_rcu_pages(p);
>  	if (TestSetPageHWPoison(p)) {
>  		res = -EHWPOISON;
>  		if (flags & MF_ACTION_REQUIRED)
> @@ -2447,6 +2462,13 @@ int memory_failure(unsigned long pfn, int flags)
>  		if (flags & MF_COUNT_INCREASED)
>  			put_page(p);
>  		action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
> +		/*
> +		 * The memory_failure() that set PG_hwpoison may have failed
> +		 * without waiting for lockless readers, and the caller may
> +		 * unmap the page once this returns 0.
> +		 */
> +		if (!res && folio_test_rcu_free(page_folio(p)))
> +			synchronize_rcu();
>  		goto unlock_mutex;
>  	}

[Severity: High]
As reported by Sashiko on RFC [3], can this already-poisoned path in
memory_failure() return 0 and allow the caller (such as kill_me_maybe() ->
set_mce_nospec()) to unmap pfn from the kernel direct map while lockless
RCU page-cache readers can still access p?

There appear to be two issues when a previous memory_failure() call set
PG_hwpoison on p and then failed before isolating the page (for example,
in get_hwpoison_page() or try_to_split_thp_page()), and a second
memory_failure(pfn, MF_ACTION_REQUIRED) call enters the
TestSetPageHWPoison(p) branch with kill_accessing_process() returning 0:

1. In the TestSetPageHWPoison(p) branch, put_page(p) drops the
MF_COUNT_INCREASED reference before folio_test_rcu_free(page_folio(p)) is
checked, and no reference is held on p during kill_accessing_process() if
MF_COUNT_INCREASED was not set. If the folio's last reference is dropped
after mf_free_rcu_pages(p) (either by put_page(p) or concurrently on
another CPU), __free_pages_prepare() immediately clears PG_rcu_free before
deferring the free:

mm/page_alloc.c:__free_pages_prepare() {
	if (unlikely(folio_test_rcu_free(folio)) &&
	    !(PageHWPoison(page) && !order)) {
		VM_WARN_ON_ONCE(fpi_flags & FPI_NOLOCK);
		__folio_clear_rcu_free(folio);
		count_vm_events(PGFREE_RCU, 1 << order);
		rcu_free_defer(folio);
		return false;
	}
}

When that happens, folio_test_rcu_free(page_folio(p)) evaluates to false
and skips synchronize_rcu() while the folio is still waiting for its RCU
grace period and lockless readers that looked up the folio before it was
freed are still copying from p.

2. If p is part of a large folio and the first memory_failure() failed in
get_hwpoison_page() (before folio_set_has_hwpoisoned(folio)) or in
try_to_split_thp_page():

mm/memory-failure.c:memory_failure() {
		if (err || new_order) {
			/* get folio again in case the original one is split */
			folio = page_folio(p);
			res = -EHWPOISON;
			kill_procs_now(p, pfn, flags, folio);
			put_page(p);
			action_result(pfn, MF_MSG_UNSPLIT_THP, MF_FAILED);
			goto unlock_mutex;
		}
}

the unsplit large folio remains in the page cache with PG_hwpoison only
set on subpage p. Even after synchronize_rcu() completes, new lockless
page-cache readers can still copy the poisoned subpage as part of the
large folio while the caller unmaps p from the direct map.

[1] https://lore.kernel.org/r/20261019154913.1765974-9-kas@kernel.org
[2] https://lore.kernel.org/r/20261019154913.1765974-9-kas@kernel.org
[3] https://lore.kernel.org/r/20261019154913.1765974-9-kas@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104130118.2507513-1-kas@kernel.org?part=9