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" <[email protected]> >>>>> 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....@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
