Re: [PATCH 0/2] mm: memory-failure: fix HWPoison flag race with non-atomic page flag ops

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 29, 2026 at 08:49:37AM +0200, David Hildenbrand (Arm) wrote:
> On 6/28/26 23:45, Michael S. Tsirkin wrote:
> > I don't like it that we are adding overhead to the good path for
> > the benefit of memory failure, which never triggers on many systems,
> > but I don't have a better idea. Pls take a look.
> 
> As I said on Friday.
> 
> "It's also doesn't address the mf_mutex implications and the x86 thingies I
> mentioned.

Well I did attempt addressing this.  These would be these two:

	(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.

page_test_set_hwpoison was this code you wrote:

+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();
+       }
+}

and indeed the test+set combination seems racy.  But consider the version I posted, for example:

+/*
+ * Drain any in-flight non-atomic page flag operations that could
+ * clobber a concurrently set HWPoison bit.  Retries until the bit sticks.
+ */
+static void set_hwpoison_drain_rcu(struct page *p)
+{
+       do {
+               synchronize_rcu();
+       } while (!TestSetPageHWPoison(p));
+}
+

...

+static bool test_and_set_hwpoison_drain_rcu(struct page *p)
+{
+       bool was_set = TestSetPageHWPoison(p);
+
+       set_hwpoison_drain_rcu(p);
+       return was_set;
+}



does not seem racy without a lock. But maybe I don't get it.



	(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.

Well, I did try to document the situation - it's in the commit log for
patch 1:

    Note: the MCE handler in arch/x86/kernel/cpu/mce/core.c also calls
    SetPageHWPoison() and is subject to the same race.  It cannot use
    the drain helpers (MCE context cannot call synchronize_rcu()).
    For recoverable MCE errors, memory_failure() is queued via work
    items (kill_me_maybe/kill_me_never) and will re-set the bit via
    test_and_set_hwpoison_drain_rcu() if it was clobbered.  The
    mce_panic() path sets HWPoison for kdump right before panic() so
    the race is irrelevant there.  The MCG_STATUS_SEAM_NR path does
    not queue memory_failure(), but the affected page belongs to a
    TDX guest whose CPU core has already been marked dead - the page
    is not subject to concurrent non-atomic flag operations in the
    buddy allocator, so the race does not apply.






> ...
> 
> I'll either take care of that myself or find someone that can work on this with
> attention to all details.
> "

> 
> This is nothing to vibe-code. This needs a real expert.

Well I had this sitting on the disk anyway, so I thought I'd post.

I wouldn't call this vibe-code - a bunch of manual work went into this,
llms mostly as a grep/sed replacement.  But hey.  I don't object to
someone taking over, for sure. Was fun, and maybe these patches will be
helpful as a starting point.

In particular, maybe I should have been more explicit about how your
points from Friday are addressed.

If you want to add a bit more to explain the exact concerns here, for
whoever works on this next, feel free to do so.




> > 
> > Non-atomic page flag operations (page->flags.f &= ~mask, __set_bit,
> > __clear_bit) can race with atomic TestSetPageHWPoison() in
> > memory_failure().  The non-atomic RMW reads flags, memory_failure()
> > atomically sets HWPoison, then the RMW writes back the old value
> > without HWPoison, clobbering the bit.
> > 
> > The race was confirmed by injecting a cpu_relax() delay between the
> > load and store of the non-atomic RMW in __free_pages_prepare, then
> > running concurrent MADV_HWPOISON injection.  The clobbered HWPoison
> > bit was observed repeatedly.
> > 
> > This series fixes the race by:
> > 
> > 1. Having memory_failure() call synchronize_rcu() + retry after
> >    setting HWPoison, so that any in-flight non-atomic RMW that
> >    read the old flags value completes before we proceed.
> > 
> > 2. Wrapping all non-atomic page flag operations in
> >    rcu_read_lock/rcu_read_unlock (CONFIG_MEMORY_FAILURE only),
> >    so that synchronize_rcu() actually drains them.
> > 
> > Performance impact (page alloc+free microbenchmark, 200K iterations,
> > 20 runs, KVM guest, error bars are 3-sigma):
> > 
> >   !PREEMPT_RCU (x86):
> >                          insns/iter      cycles/iter
> >     base:                12237 +/- 1     17954 +/- 136
> >     patched:             +22 +/- 1       -124 +/- 122
> >                          (+0.18%)        (within noise)
> > 
> >   PREEMPT_RCU:
> >                          insns/iter      cycles/iter
> >     base:                12512 +/- 3     18541 +/- 214
> >     patched:             +95 +/- 3       -12 +/- 161
> >                          (+0.76%)        (within noise)
> > 
> > When !CONFIG_MEMORY_FAILURE, all wrappers compile away completely.
> > 
> > Suggested-by: David Hildenbrand <david@redhat.com>
> 
> No ;)
> 
> -- 
> Cheers,
> 
> David