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

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

[Michael S. Tsirkin]

TestSetPageHWPoison() is called without zone->lock, so its atomic
update to page->flags can race with non-atomic flag operations
that run under zone->lock in the buddy allocator.

In particular, __free_pages_prepare() does:

    page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;

This non-atomic read-modify-write, while correctly excluding
__PG_HWPOISON from the mask, can still lose a concurrent
TestSetPageHWPoison if the read happens before the poison bit
is set and the write happens after.  Will only get worse if/when
we add more non-atomic flag operations.

Fix by acquiring zone->lock around TestSetPageHWPoison and
around ClearPageHWPoison in the retry path.  This
serializes with all buddy flag manipulation.  The cost is
negligible: one lock/unlock in an extremely rare path
(hardware memory errors).

Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
in this file operate on pages already removed from the buddy
allocator or on non-buddy pages (DAX, hugetlb), so they do not
need zone->lock protection.

Fixes: 6a46079cf57a ("HWPOISON: The high level memory error handler in the VM v7")
Acked-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: Claude:claude-opus-4-6
---

Sending separately as suggested by multiple people. I also added
a Fixes tag.


 mm/memory-failure.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index ee42d4361309..3880486028a1 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2348,6 +2348,8 @@ int memory_failure(unsigned long pfn, int flags)
 	unsigned long page_flags;
 	bool retry = true;
 	int hugetlb = 0;
+	struct zone *zone;
+	unsigned long mf_flags;
 
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
@@ -2390,7 +2392,11 @@ int memory_failure(unsigned long pfn, int flags)
 	if (hugetlb)
 		goto unlock_mutex;
 
+	/* Serialize with non-atomic buddy flag operations */
+	zone = page_zone(p);
+	spin_lock_irqsave(&zone->lock, mf_flags);
 	if (TestSetPageHWPoison(p)) {
+		spin_unlock_irqrestore(&zone->lock, mf_flags);
 		res = -EHWPOISON;
 		if (flags & MF_ACTION_REQUIRED)
 			res = kill_accessing_process(current, pfn, flags);
@@ -2399,6 +2405,7 @@ int memory_failure(unsigned long pfn, int flags)
 		action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
 		goto unlock_mutex;
 	}
+	spin_unlock_irqrestore(&zone->lock, mf_flags);
 
 	/*
 	 * We need/can do nothing about count=0 pages.
@@ -2420,7 +2427,10 @@ int memory_failure(unsigned long pfn, int flags)
 			} else {
 				/* We lost the race, try again */
 				if (retry) {
+					/* Serialize with non-atomic buddy flag operations */
+					spin_lock_irqsave(&zone->lock, mf_flags);
 					ClearPageHWPoison(p);
+					spin_unlock_irqrestore(&zone->lock, mf_flags);
 					retry = false;
 					goto try_again;
 				}
-- 
MST

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

[David Hildenbrand (Arm)]

On 6/9/26 12:12, Michael S. Tsirkin wrote:
> TestSetPageHWPoison() is called without zone->lock, so its atomic
> update to page->flags can race with non-atomic flag operations
> that run under zone->lock in the buddy allocator.
> 
> In particular, __free_pages_prepare() does:
> 
>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> 
> This non-atomic read-modify-write, while correctly excluding
> __PG_HWPOISON from the mask, can still lose a concurrent
> TestSetPageHWPoison if the read happens before the poison bit
> is set and the write happens after.  Will only get worse if/when
> we add more non-atomic flag operations.
> 
> Fix by acquiring zone->lock around TestSetPageHWPoison and
> around ClearPageHWPoison in the retry path.  This
> serializes with all buddy flag manipulation.  The cost is
> negligible: one lock/unlock in an extremely rare path
> (hardware memory errors).
> 
> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> in this file operate on pages already removed from the buddy
> allocator or on non-buddy pages (DAX, hugetlb), so they do not
> need zone->lock protection.
> 
> Fixes: 6a46079cf57a ("HWPOISON: The high level memory error handler in the VM v7")
> Acked-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Assisted-by: Claude:claude-opus-4-6
> ---

memory_failure() is documented that it must not be called with the spinlock
held, so this cannot deadlock. In particular, we would grab the lock already in
take_page_off_buddy().

LGTM, thanks

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David

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

[Zi Yan]

On 9 Jun 2026, at 6:12, Michael S. Tsirkin wrote:

> TestSetPageHWPoison() is called without zone->lock, so its atomic
> update to page->flags can race with non-atomic flag operations
> that run under zone->lock in the buddy allocator.
>
> In particular, __free_pages_prepare() does:
>
>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>
> This non-atomic read-modify-write, while correctly excluding
> __PG_HWPOISON from the mask, can still lose a concurrent
> TestSetPageHWPoison if the read happens before the poison bit
> is set and the write happens after.  Will only get worse if/when
> we add more non-atomic flag operations.
>
> Fix by acquiring zone->lock around TestSetPageHWPoison and
> around ClearPageHWPoison in the retry path.  This
> serializes with all buddy flag manipulation.  The cost is
> negligible: one lock/unlock in an extremely rare path
> (hardware memory errors).
>
> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> in this file operate on pages already removed from the buddy
> allocator or on non-buddy pages (DAX, hugetlb), so they do not
> need zone->lock protection.
>
> Fixes: 6a46079cf57a ("HWPOISON: The high level memory error handler in the VM v7")
> Acked-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Assisted-by: Claude:claude-opus-4-6
> ---
>
> Sending separately as suggested by multiple people. I also added
> a Fixes tag.
>
>
>  mm/memory-failure.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
Makes sense to me. Thanks.

Acked-by: Zi Yan <ziy@nvidia.com>

Best Regards,
Yan, Zi

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

[Andrew Morton]

On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:

> TestSetPageHWPoison() is called without zone->lock, so its atomic
> update to page->flags can race with non-atomic flag operations
> that run under zone->lock in the buddy allocator.
> 
> In particular, __free_pages_prepare() does:
> 
>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> 
> This non-atomic read-modify-write, while correctly excluding
> __PG_HWPOISON from the mask, can still lose a concurrent
> TestSetPageHWPoison if the read happens before the poison bit
> is set and the write happens after.  Will only get worse if/when
> we add more non-atomic flag operations.
> 
> Fix by acquiring zone->lock around TestSetPageHWPoison and
> around ClearPageHWPoison in the retry path.  This
> serializes with all buddy flag manipulation.  The cost is
> negligible: one lock/unlock in an extremely rare path
> (hardware memory errors).
> 
> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> in this file operate on pages already removed from the buddy
> allocator or on non-buddy pages (DAX, hugetlb), so they do not
> need zone->lock protection.

Sashiko is saying this doesn't do anything "Because
__free_pages_prepare() executes entirely locklessly".  Did it goof?

https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com

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

[David Hildenbrand (Arm)]

On 6/9/26 20:10, Andrew Morton wrote:
> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> TestSetPageHWPoison() is called without zone->lock, so its atomic
>> update to page->flags can race with non-atomic flag operations
>> that run under zone->lock in the buddy allocator.
>>
>> In particular, __free_pages_prepare() does:
>>
>>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>
>> This non-atomic read-modify-write, while correctly excluding
>> __PG_HWPOISON from the mask, can still lose a concurrent
>> TestSetPageHWPoison if the read happens before the poison bit
>> is set and the write happens after.  Will only get worse if/when
>> we add more non-atomic flag operations.
>>
>> Fix by acquiring zone->lock around TestSetPageHWPoison and
>> around ClearPageHWPoison in the retry path.  This
>> serializes with all buddy flag manipulation.  The cost is
>> negligible: one lock/unlock in an extremely rare path
>> (hardware memory errors).
>>
>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
>> in this file operate on pages already removed from the buddy
>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
>> need zone->lock protection.
> 
> Sashiko is saying this doesn't do anything "Because
> __free_pages_prepare() executes entirely locklessly".  Did it goof?
> 
> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com

Battle of the bots: it's right.

-- 
Cheers,

David

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

[Zi Yan]

On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:

> On 6/9/26 20:10, Andrew Morton wrote:
>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
>>> update to page->flags can race with non-atomic flag operations
>>> that run under zone->lock in the buddy allocator.
>>>
>>> In particular, __free_pages_prepare() does:
>>>
>>>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>
>>> This non-atomic read-modify-write, while correctly excluding
>>> __PG_HWPOISON from the mask, can still lose a concurrent
>>> TestSetPageHWPoison if the read happens before the poison bit
>>> is set and the write happens after.  Will only get worse if/when
>>> we add more non-atomic flag operations.
>>>
>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
>>> around ClearPageHWPoison in the retry path.  This
>>> serializes with all buddy flag manipulation.  The cost is
>>> negligible: one lock/unlock in an extremely rare path
>>> (hardware memory errors).
>>>
>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
>>> in this file operate on pages already removed from the buddy
>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
>>> need zone->lock protection.
>>
>> Sashiko is saying this doesn't do anything "Because
>> __free_pages_prepare() executes entirely locklessly".  Did it goof?
>>
>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
>
> Battle of the bots: it's right.

Yep, __free_pages_prepare() changes the page flag without holding
zone->lock.

Best Regards,
Yan, Zi

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

[Zi Yan]

On 9 Jun 2026, at 14:39, Zi Yan wrote:

> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
>
>> On 6/9/26 20:10, Andrew Morton wrote:
>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>
>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
>>>> update to page->flags can race with non-atomic flag operations
>>>> that run under zone->lock in the buddy allocator.
>>>>
>>>> In particular, __free_pages_prepare() does:
>>>>
>>>>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>
>>>> This non-atomic read-modify-write, while correctly excluding
>>>> __PG_HWPOISON from the mask, can still lose a concurrent
>>>> TestSetPageHWPoison if the read happens before the poison bit
>>>> is set and the write happens after.  Will only get worse if/when
>>>> we add more non-atomic flag operations.
>>>>
>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
>>>> around ClearPageHWPoison in the retry path.  This
>>>> serializes with all buddy flag manipulation.  The cost is
>>>> negligible: one lock/unlock in an extremely rare path
>>>> (hardware memory errors).
>>>>
>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
>>>> in this file operate on pages already removed from the buddy
>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
>>>> need zone->lock protection.
>>>
>>> Sashiko is saying this doesn't do anything "Because
>>> __free_pages_prepare() executes entirely locklessly".  Did it goof?
>>>
>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
>>
>> Battle of the bots: it's right.
>
> Yep, __free_pages_prepare() changes the page flag without holding
> zone->lock.

__free_pages_prepare() works on frozen pages and assumes no one else
touches the input page. To avoid this race, memory_failure() might
want to try_get_page() before TestClearPageHWPoison(), but I am not
sure if that works along with memory failure flow.

Best Regards,
Yan, Zi

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

[Michael S. Tsirkin]

On Tue, Jun 09, 2026 at 11:10:20AM -0700, Andrew Morton wrote:
> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > TestSetPageHWPoison() is called without zone->lock, so its atomic
> > update to page->flags can race with non-atomic flag operations
> > that run under zone->lock in the buddy allocator.
> > 
> > In particular, __free_pages_prepare() does:
> > 
> >     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> > 
> > This non-atomic read-modify-write, while correctly excluding
> > __PG_HWPOISON from the mask, can still lose a concurrent
> > TestSetPageHWPoison if the read happens before the poison bit
> > is set and the write happens after.  Will only get worse if/when
> > we add more non-atomic flag operations.
> > 
> > Fix by acquiring zone->lock around TestSetPageHWPoison and
> > around ClearPageHWPoison in the retry path.  This
> > serializes with all buddy flag manipulation.  The cost is
> > negligible: one lock/unlock in an extremely rare path
> > (hardware memory errors).
> > 
> > Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> > in this file operate on pages already removed from the buddy
> > allocator or on non-buddy pages (DAX, hugetlb), so they do not
> > need zone->lock protection.
> 
> Sashiko is saying this doesn't do anything "Because
> __free_pages_prepare() executes entirely locklessly".  Did it goof?
> 
> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
>

Oh. So it only helps with the prezero patches. Maybe other places where
flags are touched locklessly. Not __free_pages_prepare. I was too
focused on that. Scrap this please. I'll try to think of something.


-- 
MST

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

[Michael S. Tsirkin]

On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote:
> On 9 Jun 2026, at 14:39, Zi Yan wrote:
> 
> > On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
> >
> >> On 6/9/26 20:10, Andrew Morton wrote:
> >>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>
> >>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
> >>>> update to page->flags can race with non-atomic flag operations
> >>>> that run under zone->lock in the buddy allocator.
> >>>>
> >>>> In particular, __free_pages_prepare() does:
> >>>>
> >>>>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> >>>>
> >>>> This non-atomic read-modify-write, while correctly excluding
> >>>> __PG_HWPOISON from the mask, can still lose a concurrent
> >>>> TestSetPageHWPoison if the read happens before the poison bit
> >>>> is set and the write happens after.  Will only get worse if/when
> >>>> we add more non-atomic flag operations.
> >>>>
> >>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
> >>>> around ClearPageHWPoison in the retry path.  This
> >>>> serializes with all buddy flag manipulation.  The cost is
> >>>> negligible: one lock/unlock in an extremely rare path
> >>>> (hardware memory errors).
> >>>>
> >>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> >>>> in this file operate on pages already removed from the buddy
> >>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
> >>>> need zone->lock protection.
> >>>
> >>> Sashiko is saying this doesn't do anything "Because
> >>> __free_pages_prepare() executes entirely locklessly".  Did it goof?
> >>>
> >>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
> >>
> >> Battle of the bots: it's right.
> >
> > Yep, __free_pages_prepare() changes the page flag without holding
> > zone->lock.
> 
> __free_pages_prepare() works on frozen pages and assumes no one else
> touches the input page. To avoid this race, memory_failure() might
> want to try_get_page() before TestClearPageHWPoison(), but I am not
> sure if that works along with memory failure flow.
> 
> Best Regards,
> Yan, Zi



Actually memory failure already plays with this down the road no?

So maybe it's enough to just SetPageHWPoison afterwards again?


diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index ee42d4361309..4758fea94a96 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!res) {
 		if (is_free_buddy_page(p)) {
 			if (take_page_off_buddy(p)) {
+				SetPageHWPoison(p);
 				page_ref_inc(p);
 				res = MF_RECOVERED;
 			} else {


and maybe in a bunch of other places in there?

-- 
MST

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

[Zi Yan]

On 9 Jun 2026, at 16:34, Michael S. Tsirkin wrote:

> On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote:
>> On 9 Jun 2026, at 14:39, Zi Yan wrote:
>>
>>> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
>>>
>>>> On 6/9/26 20:10, Andrew Morton wrote:
>>>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>>
>>>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
>>>>>> update to page->flags can race with non-atomic flag operations
>>>>>> that run under zone->lock in the buddy allocator.
>>>>>>
>>>>>> In particular, __free_pages_prepare() does:
>>>>>>
>>>>>>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>>>
>>>>>> This non-atomic read-modify-write, while correctly excluding
>>>>>> __PG_HWPOISON from the mask, can still lose a concurrent
>>>>>> TestSetPageHWPoison if the read happens before the poison bit
>>>>>> is set and the write happens after.  Will only get worse if/when
>>>>>> we add more non-atomic flag operations.
>>>>>>
>>>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
>>>>>> around ClearPageHWPoison in the retry path.  This
>>>>>> serializes with all buddy flag manipulation.  The cost is
>>>>>> negligible: one lock/unlock in an extremely rare path
>>>>>> (hardware memory errors).
>>>>>>
>>>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
>>>>>> in this file operate on pages already removed from the buddy
>>>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
>>>>>> need zone->lock protection.
>>>>>
>>>>> Sashiko is saying this doesn't do anything "Because
>>>>> __free_pages_prepare() executes entirely locklessly".  Did it goof?
>>>>>
>>>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
>>>>
>>>> Battle of the bots: it's right.
>>>
>>> Yep, __free_pages_prepare() changes the page flag without holding
>>> zone->lock.
>>
>> __free_pages_prepare() works on frozen pages and assumes no one else
>> touches the input page. To avoid this race, memory_failure() might
>> want to try_get_page() before TestClearPageHWPoison(), but I am not
>> sure if that works along with memory failure flow.
>>
>> Best Regards,
>> Yan, Zi
>
>
>
> Actually memory failure already plays with this down the road no?
>
> So maybe it's enough to just SetPageHWPoison afterwards again?
>
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index ee42d4361309..4758fea94a96 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags)
>  	if (!res) {
>  		if (is_free_buddy_page(p)) {
>  			if (take_page_off_buddy(p)) {
> +				SetPageHWPoison(p);
>  				page_ref_inc(p);
>  				res = MF_RECOVERED;
>  			} else {
>
>
> and maybe in a bunch of other places in there?

You mean for fear of losing HWPoison flag in the earlier TestSetPageHWPoison(),
just set it again here? Why not do it after get_hwpoison_page(), since that
is the expected page flag? Miaohe probably can give a better answer here.


Best Regards,
Yan, Zi

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

[Michael S. Tsirkin]

On Tue, Jun 09, 2026 at 04:54:01PM -0400, Zi Yan wrote:
> On 9 Jun 2026, at 16:34, Michael S. Tsirkin wrote:
> 
> > On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote:
> >> On 9 Jun 2026, at 14:39, Zi Yan wrote:
> >>
> >>> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
> >>>
> >>>> On 6/9/26 20:10, Andrew Morton wrote:
> >>>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>>>
> >>>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
> >>>>>> update to page->flags can race with non-atomic flag operations
> >>>>>> that run under zone->lock in the buddy allocator.
> >>>>>>
> >>>>>> In particular, __free_pages_prepare() does:
> >>>>>>
> >>>>>>     page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> >>>>>>
> >>>>>> This non-atomic read-modify-write, while correctly excluding
> >>>>>> __PG_HWPOISON from the mask, can still lose a concurrent
> >>>>>> TestSetPageHWPoison if the read happens before the poison bit
> >>>>>> is set and the write happens after.  Will only get worse if/when
> >>>>>> we add more non-atomic flag operations.
> >>>>>>
> >>>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
> >>>>>> around ClearPageHWPoison in the retry path.  This
> >>>>>> serializes with all buddy flag manipulation.  The cost is
> >>>>>> negligible: one lock/unlock in an extremely rare path
> >>>>>> (hardware memory errors).
> >>>>>>
> >>>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> >>>>>> in this file operate on pages already removed from the buddy
> >>>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
> >>>>>> need zone->lock protection.
> >>>>>
> >>>>> Sashiko is saying this doesn't do anything "Because
> >>>>> __free_pages_prepare() executes entirely locklessly".  Did it goof?
> >>>>>
> >>>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
> >>>>
> >>>> Battle of the bots: it's right.
> >>>
> >>> Yep, __free_pages_prepare() changes the page flag without holding
> >>> zone->lock.
> >>
> >> __free_pages_prepare() works on frozen pages and assumes no one else
> >> touches the input page. To avoid this race, memory_failure() might
> >> want to try_get_page() before TestClearPageHWPoison(), but I am not
> >> sure if that works along with memory failure flow.
> >>
> >> Best Regards,
> >> Yan, Zi
> >
> >
> >
> > Actually memory failure already plays with this down the road no?
> >
> > So maybe it's enough to just SetPageHWPoison afterwards again?
> >
> >
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index ee42d4361309..4758fea94a96 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags)
> >  	if (!res) {
> >  		if (is_free_buddy_page(p)) {
> >  			if (take_page_off_buddy(p)) {
> > +				SetPageHWPoison(p);
> >  				page_ref_inc(p);
> >  				res = MF_RECOVERED;
> >  			} else {
> >
> >
> > and maybe in a bunch of other places in there?
> 
> You mean for fear of losing HWPoison flag in the earlier TestSetPageHWPoison(),
> just set it again here?

Yea.

> Why not do it after get_hwpoison_page(), since that
> is the expected page flag?

It's still in the buddy at that point right? I'm worried buddy might
poke at flags.

> Miaohe probably can give a better answer here.
> 
> 
> Best Regards,
> Yan, Zi