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

[Zi Yan <ziy@nvidia.com>]

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_*().
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.
3. we cannot take a ref on slab page and other frozen pages, then we will
   need a case by case handling for them.

Anyway, this probably belongs a separate discussion on hwpoison.

Best Regards,
Yan, Zi