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

[David Hildenbrand (Arm)]

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?

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.

-- 
Cheers,

David