Re: [PATCH splitout] mm: memory-failure: serialize TestSetPageHWPoison with zone->lock

["David Hildenbrand (Arm)" <david@kernel.org>]

On 6/15/26 05:29, Miaohe Lin wrote:
> On 2026/6/11 21:20, David Hildenbrand (Arm) wrote:
>> On 6/11/26 09:36, Miaohe Lin wrote:
>>>
>>> Agree, it's not worth to do so.
>>>
>>>
>>> Since memory_failure might be the only place, this change would be unacceptable.
>>> We should come up with a better solution. Maybe we can try repeating SetPageHWPoison
>>> and ClearPageHWPoison at a first attempt though it looks somewhat weird to me and makes
>>> code more complicated.
>>
>> And I am fairly sure we could still have some remaining races ... it's shaky.
> 
> I have to agree it's shaky.

Right, just let writing task reschedule after reading the flags,
but before writing the flags.

> Any suggestion for next step?

We have various code that assumes that no concurrent writes are
possible, and consequently, we use no atomics.

__free_pages_prepare() is just one user.

Then we have __folio_set_locked(), __folio_clear_active()
and __folio_clear_unevictable().

But also __folio_mark_uptodate(), which is called rather frequently.

page_cpupid_reset_last() is also a thing, but it mostly falls
under __free_pages_prepare() handling.

... and __split_folio_to_order() also messes with flags directly without atomics.


Many of these are only possible for frozen pages (refcount == 0). I think
only  __folio_set_locked() and __folio_mark_uptodate() are called on
non-frozen pages, when there is the expectation that nobody will concurrently
use atomics that would be bad (e.g., don't trylock if not an lru page).


We don't want to use atomics at these places just to please memory failure code.

Would it be sufficient to know in memory-failure code that concurrent
handling succeeded?


Assume that we enlighten all non-atomics to grab the rcu read lock, such as

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 7223f6f4e2b4..3c3852b60bbd 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -803,10 +803,30 @@ static inline bool PageUptodate(const struct page *page)
        return folio_test_uptodate(page_folio(page));
 }
 
+#ifdef CONFIG_MEMORY_FAILURE
+static inline void page_flags_modify_nonatomic_begin(void)
+{
+       rcu_read_lock();
+}
+static inline void page_flags_modify_nonatomic_end(void)
+{
+       rcu_read_unlock();
+}
+#else
+static inline void page_flags_modify_nonatomic_begin(void)
+{
+}
+static inline void page_flags_modify_nonatomic_end(void)
+{
+}
+#endif
+
 static __always_inline void __folio_mark_uptodate(struct folio *folio)
 {
        smp_wmb();
+       page_flags_modify_nonatomic_begin();
        __set_bit(PG_uptodate, folio_flags(folio, 0));
+       page_flags_modify_nonatomic_end();
 }
 

And then we have some retry logic such as:

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 51508a55c405..1123c40aaf43 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -162,6 +162,62 @@ static struct rb_root_cached pfn_space_itree = RB_ROOT_CACHED;
 
 static DEFINE_MUTEX(pfn_space_lock);
 
+static bool page_test_set_hwpoison(struct page *page)
+{
+	lockdep_assert_held(&mf_mutex);
+
+	while (true) {
+		/* Already set -> not our problem. */
+		if (TestSetPageHWPoison(page))
+			return true;
+		/* Make sure concurrent non-atomic writers completed. */
+		synchronize_rcu();
+		/* Setting the flag was sticky. */
+		if (PageHWPoison(page))
+			return false;
+	}
+}
+
+static bool page_test_clear_hwpoison(struct page *page)
+{
+	lockdep_assert_held(&mf_mutex);
+
+	while (true) {
+		/* Already clear -> not our problem. */
+		if (!TestClearPageHWPoison(page))
+			return false;
+		/* Make sure concurrent non-atomic writers completed. */
+		synchronize_rcu();
+		/* Clearing the flag was sticky. */
+		if (!PageHWPoison(page))
+			return true;
+	}
+}
+
+static void page_set_hwpoison(struct page *page)
+{
+	lockdep_assert_held(&mf_mutex);
+
+	while (!PageHWPoison(page)) {
+		SetPageHWPoison(page);
+
+		/* Make sure concurrent non-atomic writers completed. */
+		synchronize_rcu();
+	}
+}
+
+static void page_clear_hwpoison(struct page *page)
+{
+	lockdep_assert_held(&mf_mutex);
+
+	while (PageHWPoison(page)) {
+		ClearPageHWPoison(page);
+
+		/* Make sure concurrent non-atomic writers completed. */
+		synchronize_rcu();
+	}
+}
+
 /*
  * Return values:
  *   1:   the page is dissolved (if needed) and taken off from buddy,
@@ -199,7 +255,7 @@ static bool page_handle_poison(struct page *page, bool hugepage_or_freepage, boo
 			return false;
 	}
 
-	SetPageHWPoison(page);
+	page_set_hwpoison(page);
 	if (release)
 		put_page(page);
 	page_ref_inc(page);
@@ -1744,7 +1800,7 @@ static int mf_generic_kill_procs(unsigned long long pfn, int flags,
 	 * Use this flag as an indication that the dax page has been
 	 * remapped UC to prevent speculative consumption of poison.
 	 */
-	SetPageHWPoison(&folio->page);
+	page_set_hwpoison(&folio->page);
 
 	/*
 	 * Unlike System-RAM there is no possibility to swap in a
@@ -1789,7 +1845,7 @@ int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
 			goto unlock;
 
 		if (!pre_remove)
-			SetPageHWPoison(page);
+			page_set_hwpoison(page);
 
 		/*
 		 * The pre_remove case is revoking access, the memory is still
@@ -1866,7 +1922,7 @@ static unsigned long __folio_free_raw_hwp(struct folio *folio, bool move_flag)
 	head = llist_del_all(raw_hwp_list_head(folio));
 	llist_for_each_entry_safe(p, next, head, node) {
 		if (move_flag)
-			SetPageHWPoison(p->page);
+			page_set_hwpoison(p->page);
 		else
 			num_poisoned_pages_sub(page_to_pfn(p->page), 1);
 		kfree(p);
@@ -2380,7 +2436,7 @@ int memory_failure(unsigned long pfn, int flags)
 	if (res != -ENOENT)
 		goto unlock_mutex;
 
-	if (TestSetPageHWPoison(p)) {
+	if (page_test_set_hwpoison(p)) {
 		res = -EHWPOISON;
 		if (flags & MF_ACTION_REQUIRED)
 			res = kill_accessing_process(current, pfn, flags);
@@ -2410,7 +2466,7 @@ int memory_failure(unsigned long pfn, int flags)
 			} else {
 				/* We lost the race, try again */
 				if (retry) {
-					ClearPageHWPoison(p);
+					page_clear_hwpoison(p);
 					retry = false;
 					goto try_again;
 				}
@@ -2431,7 +2487,7 @@ int memory_failure(unsigned long pfn, int flags)
 	/* filter pages that are protected from hwpoison test by users */
 	folio_lock(folio);
 	if (hwpoison_filter(p)) {
-		ClearPageHWPoison(p);
+		page_clear_hwpoison(p);
 		folio_unlock(folio);
 		folio_put(folio);
 		res = -EOPNOTSUPP;
@@ -2751,7 +2807,7 @@ int unpoison_memory(unsigned long pfn)
 		}
 
 		folio_put(folio);
-		if (TestClearPageHWPoison(p)) {
+		if (page_test_clear_hwpoison(p)) {
 			folio_put(folio);
 			ret = 0;
 		}


Maybe that would work. There would still be issues to solve

(a) We don't hold the mf_mutex on all call paths, but we really need it so a
page_test_set_hwpoison() cannot race in weird ways with the other primitives I think.

(b) There are some leftover SetPageHWPoison etc. instances. The ones in
arch/x86/kernel/cpu/mce/core.c likely cannot grab the mutex, but maybe they are
corner cases either way and we can document the situation.


Further, while I assume the synchronize_rcu() on the MCE path should be fine
(who cares about performance there?), I don't know if the added RCU read lock
on some paths could be noticable.

So one idea worth discussing, but I am sure there are more problems.

-- 
Cheers,

David