Re: [PATCH v3] mm/huge_memory: prevent losing PG_waiters in __split_folio_to_order()

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 11/10/26 18:03, Zi Yan wrote:
> On 10 Nov 2026, at 6:31, David Hildenbrand (Arm) wrote:
> 
>> On 11/9/26 22:05, Zi Yan wrote:
>>> On 9 Nov 2026, at 15:21, David Hildenbrand (Arm) wrote:
>>>
>>>> On 11/9/26 20:32, Zi Yan wrote:
>>>>> __split_folio_to_order() calls prep_compound_page() on each new head page
>>>>> to create a compound page after clear_compound_head(). prep_compound_page()
>>>>> uses non atomic __SetPageHead(). It is mostly OK, since the folio is locked
>>>>> and frozen and no one else is supposed to modify the folio's flags.
>>>>>
>>>>> However, code calling migration_entry_wait() can reach the folio by PFN,
>>>>> then access its flags. It is because softleaf_entry_wait_on_locked() used
>>>>> by migration_entry_wait() sets PG_waiters concurrently without taking a
>>>>> reference and the frozen refcount during split cannot block it. Meanwhile,
>>>>> __SetPageHead() does a plain read and write, so PG_waiters atomically set
>>>>> by softleaf_entry_wait_on_locked() will be lost if the set happens after
>>>>> __SetPageHead()'s read and before its write.
>>>>>
>>>>> One example code interleaving:
>>>>>
>>>>> CPU0: __split_folio_to_order(new_order>0)  CPU1: do_swap_page()
>>>>>
>>>>> 1. clear_compound_head(new_head)
>>>>>    new_head->compound_info = 0
>>>>>    (page_folio(new_head) == new_head now)
>>>>>
>>>>>                                            2. migration_entry_wait()
>>>>>                                              softleaf_to_folio(entry)
>>>>>                                              page_folio() -> new_head
>>>>>                                              (not the old head any more)
>>>>>
>>>>> 3. prep_compound_page(new_head, new_order)
>>>>>      __SetPageHead(new_head)
>>>>>        tmp = new_head->flags.f  <-- load; PG_waiters == 0 in tmp
>>>>>
>>>>>                                            4. folio_set_waiters(new_head)
>>>>>                                              set_bit(PG_waiters, &flags.f)
>>>>>                                              enqueue on waitqueue, sleep
>>>>>
>>>>> 5.     new_head->flags.f = tmp | (1UL << PG_head)
>>>>>        <-- store; overwrites step 4, PG_waiters == 0 again
>>>>>
>>>>> Later, folio_unlock(new_folio) sees no PG_waiters, skips folio_wake_bit(),
>>>>> and CPU1 can remain blocked indefinitely.
>>>>>
>>>>> Fix it by setting PG_head along with other flags in new_folio->flags before
>>>>> smp_wmb() and clear_compound_head(). The remaining compound page metadata
>>>>> is set after clear_compound_head(). In that way, after
>>>>> clear_compound_head() makes new_folio visible, nothing overwrites the
>>>>> PG_waiters from softleaf_entry_wait_on_locked() by modifying
>>>>> new_folio->flags non-atomically.
>>>>>
>>>>> While at it, use data_race() to suppress the KCSAN issue reported by Ye
>>>>> Liu's patch (the second Closes) on reading ->flags during a folio split.
>>>>>
>>>>> Suggested-by: Kiryl Shutsemau <kas@kernel.org>
>>>>> Reported-by: David Hildenbrand <david@kernel.org>
>>>>> Closes: https://lore.kernel.org/all/DMHE8BEEER1H.2BIY52NXATPIA@nvidia.com/
>>>>> Reported-by: Ye Liu <ye.liu@linux.dev>
>>>>> Closes: https://lore.kernel.org/all/20261029-data_race_huge_mem-v1-1-112e331291c1@linux.dev/
>>>>> Fixes: c010d47f107f6 ("mm: thp: split huge page to any lower order pages")
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>>>> ---
>>>>> The issue was raised by David when reviewing Ye Liu's "mm/huge_memory: use
>>>>> READ_ONCE() when cloning folio flags during split" [1]. This patch also
>>>>> supersedes Ye Liu's patch.
>>>>>
>>>>> [1] https://lore.kernel.org/all/20261029-data_race_huge_mem-v1-1-112e331291c1@linux.dev/
>>>>> ---
>>>>> Changes in v3:
>>>>> - set PG_head before smp_wmb() like Kiryl suggested instead of
>>>>>   prep_compound_page_atomic().
>>>>> - used data_race() to suppress KCSAN reports while at it.
>>>>> - dropped Lorenzo's ack, since the patch is changed.
>>>>> - Link to v2: https://patch.msgid.link/20261106-fix-split_folio_to_order-race-v2-1-1b59c67b8af3@nvidia.com
>>>>>
>>>>> Changes in v2:
>>>>> - renamed __prep_compound_head_and_tail() to __prep_compound_page().
>>>>> - added comments to prep_compound_page() and __prep_compound_page().
>>>>> - Link to v1: https://patch.msgid.link/20261029-fix-split_folio_to_order-race-v1-1-a483c7451b0d@nvidia.com
>>>>> ---
>>>>>  mm/huge_memory.c | 32 ++++++++++++++++++++++++++++----
>>>>>  mm/internal.h    |  2 ++
>>>>>  mm/page_alloc.c  | 17 +++++++++++++++--
>>>>>  3 files changed, 45 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>> index bd44d58e86845..99b48d7c988db 100644
>>>>> --- a/mm/huge_memory.c
>>>>> +++ b/mm/huge_memory.c
>>>>> @@ -3714,6 +3714,7 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>>>>  		 * Don't pass it around before clear_compound_head().
>>>>>  		 */
>>>>>  		struct folio *new_folio = (struct folio *)new_head;
>>>>> +		memdesc_flags_t new_flags;
>>>>>
>>>>>  		VM_BUG_ON_PAGE(atomic_read(&new_folio->_mapcount) != -1, new_head);
>>>>>
>>>>> @@ -3730,8 +3731,16 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>>>>  		 * unreferenced sub-pages of an anonymous THP: we can simply drop
>>>>>  		 * PG_anon_exclusive (-> PG_mappedtodisk) for these here.
>>>>>  		 */
>>>>> -		new_folio->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>> -		new_folio->flags.f |= (folio->flags.f &
>>>>> +		/*
>>>>> +		 * ->flags are stable since the folio is locked and frozen.
>>>>> +		 * data_race() is used because reference-less writers,
>>>>> +		 * 1. PG_waiters from softleaf_entry_wait_on_locked() and
>>>>
>>>> Hm? I thought that case would be handled by your patch? How can they still write
>>>> to the page?
>>>
>>> This is V3 of the patch. Instead of using prep_compound_page_atomic(),
>>> PG_head is written before clear_compound_page(), after which new_head
>>> is visible to the reference-less writers.
>>
>> I'm confused, how is it then valid to have PG_waiters set concurrently while we
>> read new_folio->flags.f? Because that's what your comment states.
>>
>>>
>>>>
>>>>> +		 * 2. PG_hwpoison from memory_failure(),
>>>>> +		 * can still write to it.
>>>>
>>>> We lose PG_hwpoison left and right when racing with non-atomic updates. It needs
>>>> a fundamental fix. Is this really something we should take care of in this series?
>>>
>>> IMHO, memory_failure()’s reference-less access violates the frozen
>>> page contract and should be fixed.
>>
>> Exactly.
>>
>>> But I do not think it needs to go
>>> along with this patch. We discussed HWPoison flag race with
>>> Michael S. Tsirkin and Miaohe before [1]:
>>>
>>> 1. Miaohe proposed to reset HWPoison in memory_failure() as a best effort,
>>> 2. mst used RCU for all non-atomic flag accesses,
>>> 3. you also proposed to move HWPoison out of ->flags.
>>>
>>> So memory_failure() needs a separate and proper fix series after
>>> we decide the right solution.
>>>
>>
>> Vlasta raised the idea to set hwpoison only after we actually managed to
>> isolate/grab the page. Which might be the proper fix.
> 
> I was bouncing the same idea with LLM yesterday, but it does not solve
> all cases:
> 
> 1. normal page: taking a ref just prevents it from going to be a free page,
>    but atomically set PG_hwpoison can still be lost when another one uses
>    a plain write, __folio_set/clear_*().

Hm if I pin and isolate a page, I'd expect my atomic flag updates to be
RMW-race-free. Who is doing plain writes to flags on pages they don't "own"?
I'd expect there would be more breakage than hwpoison in that case.

> 2. free page: isolating works if the free page on its free list, but it
>    could be on pcplist and we then need to drain all pcplists. That could
>    hurt performance.

Sure, but it's for an event that's already disruptive enough and should not
be happening a lot.

> 3. we cannot take a ref on slab page and other frozen pages, then we will
>    need a case by case handling for them.

It's also not possible to evacuate such pages (e.g. slab) so it's just bad
luck if those got corrupted.

> Anyway, this probably belongs a separate discussion on hwpoison.
> 
> Best Regards,
> Yan, Zi