Re: [RFC PATCH 08/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: PG_rcu_free folios freed in get_any_page() are not drained on retry
- [High] mm: mf_free_rcu_pages() skips rcu_free_sync() during rcu_free_pages()
- [High] mm/memory-failure: missing synchronize_rcu() when kill_accessing_process() returns 0
--

commit 0752670541e1c89c99d63ab6b10ada844735c05b
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

mm/memory-failure: handle folios with PG_rcu_free

This commit updates memory_failure() and soft_offline_page() to handle
folios with PG_rcu_free by draining pages waiting for an RCU grace period
before attempting to grab the page, and waiting for a grace period before
returning success from memory_failure().

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 7dde7c3c9a0c5..dc66cac05b1f5 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1565,6 +1565,21 @@ static int get_hwpoison_page(struct page *p, unsigned long flags)
>  	return ret;
>  }
>  
> +/* 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 (!IS_ENABLED(CONFIG_PAGE_RCU_FREE))
> +		return;
> +	if (page_count(p))
> +		return;
> +	if (!zone_page_state_snapshot(page_zone(p), NR_FREE_RCU_PAGES))
> +		return;

[Severity: High]
Can checking NR_FREE_RCU_PAGES here cause mf_free_rcu_pages() to skip
rcu_free_sync() while a target folio is still in the middle of being
freed?

In mm/page_alloc.c:rcu_free_pages(), NR_FREE_RCU_PAGES is decremented
before free_unref_folios() runs __free_pages_prepare() and returns the
folios 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 (!folio_batch_add(&fbatch, folio)) {
			free_unref_folios(&fbatch);
			cond_resched_tasks_rcu_qs();
		}
	}
	if (folio_batch_count(&fbatch))
		free_unref_folios(&fbatch);
}

Similarly, in folios_put_refs(), folio->_refcount is decremented to 0
before __page_cache_release() and mem_cgroup_uncharge_folios() run, prior
to rcu_free_defer() incrementing NR_FREE_RCU_PAGES.

If the last RCU-freed folios in the zone are in either window when
mf_free_rcu_pages() runs on another CPU, page_count(p) is 0,
is_free_buddy_page(p) is false, and NR_FREE_RCU_PAGES is 0. That causes
mf_free_rcu_pages() to return early without calling rcu_free_sync() (which
otherwise waits on rcu_free_drain() and flushes r->rwork.work and
r->expedite).

In memory_failure(), TestSetPageHWPoison(p) then sets PG_hwpoison before
free_unref_folios() runs __free_pages_prepare(). For an order-0 folio,
__free_pages_prepare() sees PageHWPoison(page) && !order and drops the
page instead of placing it in the buddy allocator, causing
get_hwpoison_page() to see !is_free_buddy_page(p) and ignore the error
(MF_IGNORED). For a large folio entering rcu_free_defer() after
page_count(p) became 0, it is queued on rcu_free_pcpu with PG_hwpoison set
and later freed back to the buddy allocator.

> +	if (is_free_buddy_page(p))
> +		return;
> +
> +	rcu_free_sync();
> +}
> +
>  /*
>   * The caller must guarantee the folio isn't large folio, except hugetlb.
>   * try_to_unmap() can't handle it.
> @@ -2440,6 +2455,7 @@ int memory_failure(unsigned long pfn, int flags)
>  	if (res != -ENOENT)
>  		goto unlock_mutex;
>  
> +	mf_free_rcu_pages(p);
>  	if (TestSetPageHWPoison(p)) {

[Severity: High]
What happens if a folio with PG_rcu_free still has a reference when
mf_free_rcu_pages() runs (so page_count(p) > 0 returns early), and its
last reference is dropped inside get_hwpoison_page() -> get_any_page()?

Inside get_any_page(), the last reference on a PG_rcu_free folio can drop
to 0 in a few ways:
1. When MF_COUNT_INCREASED is set (for example via madvise_inject_error()),
   page_count(p) >= 1 during mf_free_rcu_pages(). If the page is not
   immediately HWPoisonHandlable() (such as !PageLRU(p) because it was
   truncated while in a per-CPU lru_add batch), get_any_page() calls
   put_page(p) and shake_page(p) and jumps to try_again.
2. Without MF_COUNT_INCREASED, if a truncated PG_rcu_free folio is still in
   a per-CPU folio_batch, __get_hwpoison_page() returns -EBUSY and
   get_any_page() calls shake_page(p) -> lru_add_drain_all(), which frees
   the folio.
3. A concurrent task drops the last reference on the folio after
   mf_free_rcu_pages() checked page_count(p), causing __get_hwpoison_page()
   to return 0 and get_any_page() to retry:

mm/memory-failure.c:get_any_page() {
			} else if (!PageHuge(p) && !is_free_buddy_page(p)) {
				/* We raced with put_page, retry. */
				if (pass++ < GET_PAGE_MAX_RETRY_NUM)
					goto try_again;
				ret = -EIO;
			}
}

When the folio is freed during any of those paths, __free_pages_prepare()
checks:

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);\n		rcu_free_defer(folio);
		return false;
	}
}

Because neither get_any_page() nor shake_page() drains RCU-freed pages on
retry, a large folio in memory_failure() (or any PG_rcu_free folio in
soft_offline_page()) is deferred via rcu_free_defer(), causing all retries
in get_any_page() to see page_count(p) == 0 and !is_free_buddy_page(p) and
fail with -EIO. In memory_failure(), PG_hwpoison is already set on p, so
when the RCU grace period later expires, the large folio is freed into the
buddy allocator with the poisoned subpage still inside it. And for an
order-0 folio in memory_failure() freed after TestSetPageHWPoison(p),
__free_pages_prepare() skips rcu_free_defer() and drops the page without
putting it in the buddy allocator, so get_any_page() also fails with -EIO
(MF_IGNORED).

>  		res = -EHWPOISON;
>  		if (flags & MF_ACTION_REQUIRED)
> @@ -2612,6 +2628,13 @@ int memory_failure(unsigned long pfn, int flags)
>  
>  identify_page_state:
>  	res = identify_page_state(pfn, p, page_flags);
> +	/*
> +	 * The caller may unmap the page once this returns 0.  Lockless
> +	 * readers that started before the page got PG_hwpoison, or before a
> +	 * split left it in a folio of its own, may still be copying it.
> +	 */
> +	if (!res && folio_test_rcu_free(folio))
> +		synchronize_rcu();
>  	mutex_unlock(&mf_mutex);
>  	return res;

[Severity: High]
Can memory_failure() return 0 without calling synchronize_rcu() when a
second memory_failure() call hits the TestSetPageHWPoison(p) path?

Consider a lockless RCU reader in filemap_read_rcu_batch() that checks
folio_contain_hwpoisoned_page(folio) before PG_hwpoison is set and begins
copying from the page via the direct map in filemap_read_rcu_copy().

1. A first memory_failure() call (for instance from a background scrubber
   or UCNA) sets PG_hwpoison via TestSetPageHWPoison(p). In
   hwpoison_user_mappings() -> unmap_poisoned_folio(), because the
   page-cache folio is clean, TTU_HWPOISON is cleared and user PTEs are
   unmapped to pte_none without installing hwpoison swap entries. If
   me_pagecache_clean() then returns MF_FAILED (e.g. due to a transient
   extra refcount in has_extra_refcount() or truncate_error_folio()
   failing), identify_page_state() returns -EBUSY, so !res is false and
   memory_failure() unlocks mf_mutex without calling synchronize_rcu().
2. A racing Action Required MCE on the same PFN calls memory_failure(pfn,
   MF_ACTION_REQUIRED). TestSetPageHWPoison(p) returns true and calls:

mm/memory-failure.c:memory_failure() {
	if (TestSetPageHWPoison(p)) {
		res = -EHWPOISON;
		if (flags & MF_ACTION_REQUIRED)
			res = kill_accessing_process(current, pfn, flags);
		if (flags & MF_COUNT_INCREASED)
			put_page(p);
		action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
		goto unlock_mutex;
	}
}

Because the clean user PTEs were already unmapped without hwpoison swap
entries, walk_page_range() in kill_accessing_process() finds no matching
PTE in current->mm and returns 0. memory_failure() then jumps to
unlock_mutex and returns 0 without waiting for an RCU grace period,
allowing the caller (such as kill_me_maybe() -> set_mce_nospec(pfn)) to
unmap the page from the kernel direct map while the lockless RCU reader is
still copying from it.

[ ... ]

>
> -- 
> 2.51.2
>

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