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

[Michael S. Tsirkin]

On Wed, Jul 08, 2026 at 02:13:27PM +0200, David Hildenbrand (Arm) wrote:
> On 7/2/26 00:18, Michael S. Tsirkin wrote:
> > On Wed, Jul 01, 2026 at 06:17:23PM +0200, David Hildenbrand (Arm) wrote:
> >> On 7/1/26 17:54, Michael S. Tsirkin wrote:
> >>>
> >>> Not "generated" surely. But assisted, yes.
> >>
> >> What I thought.
> >>
> >>> Still hacking on it, but the difficulty
> >>> with memory-failure is that fundamentally, it's not 100% robust.
> >>
> >> It's all a bit slapped on top of everything, yes.
> >>
> >> What I was wondering is, assuming the call_task_rcu() and it takes forever,
> >> there might be quite a while where a hwpoisoned page that lost its bit is not
> >> marked as hwpoisoned.
> >>
> >> So you'd actually want the one doing the test_and_set_bit() caller to wait until
> >> the bit is stable.
> > 
> > not sure I get it.
> 
> Essentially, I think that after you set the bit you'd want everybody else in the
> system to see that state before you continue.
> 
> Such that other code that tests for poisoned pages would just immediately act on
> it. Like the buddy not handing out such a page anymore, immediately.
> 
> At least that way it's easier to reason about. Losing these bits is really just
> nasty :(
> 
> > 
> >> But that should be rather hairy as well. :(
> >>
> >>>
> >>> For example, we have a fifo fed by hardware and consumed by a workqueue:
> >>>
> >>>         struct memory_failure_cpu *mf_cpu;
> >>>         unsigned long proc_flags;
> >>>         bool buffer_overflow;
> >>>         struct memory_failure_entry entry = {
> >>>                 .pfn =          pfn,
> >>>                 .flags =        flags,
> >>>         };
> >>>
> >>>         mf_cpu = &get_cpu_var(memory_failure_cpu);
> >>>         raw_spin_lock_irqsave(&mf_cpu->lock, proc_flags);
> >>>         buffer_overflow = !kfifo_put(&mf_cpu->fifo, entry);
> >>>         if (!buffer_overflow)
> >>>                 schedule_work_on(smp_processor_id(), &mf_cpu->work);
> >>>         raw_spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
> >>>         put_cpu_var(memory_failure_cpu);
> >>>         if (buffer_overflow)
> >>>                 pr_err("buffer overflow when queuing memory failure at %#lx\n",
> >>>                        pfn);
> >>>
> >>>
> >>> if there are lots of these and the scheduler is slow and it overflows,
> >>> it's sayonara you have lost the flag, right?
> >>
> >> I guess so. I assume on relevant hw you wouldn't expect a storm. But who knows :)
> >>
> >>>
> >>>
> >>> Oh and by the way, I just noticed that when buddy merges pages it does
> >>> not check the poison bit. So it looks like there's a simple way to lose
> >>> the poison bit - have it merge with a non poisoned page.
> >>
> >> When we poison, we try to take the free page off the buddy. At least that's what
> >> I remember.
> > 
> > 
> > Yes but not immediately - we set hwpoison then we try to take it off.
> > 
> > 
> >> So I think we would just then go ahead and split the free higher-order buddy
> >> page to remove the single page.
> > 
> > Later we split, yes. But I don't see where it sets HWPoison after
> > split - it calls
> > SetPageHWPoisonTakenOff but it seems to assume HWPoison is already set.
> > 
> > Am I missing something? It's late here...
> So, we set hwpoison on a page and that is sticky, even when the buddy merges
> them, right?

right. bar races but that is separate.

> When handing out pages, the buddy will check all pages through
> check_new_pages(). If the poisoned page is part of a higher-order page, that
> would still find it.
> 
> So I would assume that the hwpoison page->flags stays there, and
> PageHWPoisonTakenOff() only tells us if we succeeded in removing the page from
> the buddy ourselves.

Right, I see.  Btw in a bunch of paths even if
we unpoison we lose the page, right?
And it seems we can also lose a hugepage just because 4k in
it got poisoned?
Is that expected?

> 
> -- 
> Cheers,
> 
> David