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

[Zi Yan]

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@xxxxxxxxxx>
>> Reported-by: David Hildenbrand <david@xxxxxxxxxx>
>> Closes: https://lore.kernel.org/all/DMHE8BEEER1H.2BIY52NXATPIA@xxxxxxxxxx/
>> Reported-by: Ye Liu <ye.liu@xxxxxxxxx>
>> Closes: https://lore.kernel.org/all/20261029-data_race_huge_mem-v1-1-112e331291c1@xxxxxxxxx/
>> Fixes: c010d47f107f6 ("mm: thp: split huge page to any lower order pages")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxx/
>> ---
>> 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@xxxxxxxxxx
>>
>> 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@xxxxxxxxxx
>> ---
>>  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.

>
>> +		 * 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. 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.

[1] https://lore.kernel.org/all/cover.1782676497.git.mst@xxxxxxxxxx/

>
>> +		 */
>> +		new_flags.f = data_race(new_folio->flags.f) & ~PAGE_FLAGS_CHECK_AT_PREP;
>> +		/* See above for data_race() use */
>> +		new_flags.f |= data_race(folio->flags.f) &
>>  				((1L << PG_referenced) |
>>  				 (1L << PG_swapbacked) |
>>  				 (1L << PG_swapcache) |
>> @@ -3749,7 +3758,18 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>  #endif
>>  				 (1L << PG_dirty) |
>>  				 (1L << PG_dropbehind) |
>> -				 LRU_GEN_MASK | LRU_REFS_MASK));
>> +				 LRU_GEN_MASK | LRU_REFS_MASK);
>
> -- 
> Cheers,
>
> David


Best Regards,
Yan, Zi