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

[Zi Yan]

On 9 Nov 2026, at 10:26, Kiryl Shutsemau wrote:

> On Fri, Nov 06, 2026 at 01:28:46PM -0500, 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 adding the atomic version SetPageHead() and
>> prep_compound_page_atomic() using it. __split_folio_to_order() uses
>> prep_compound_page_atomic() to avoid the issue without adding an atomic ops
>> to other prep_compound_page() users.
>>
>> Reported-by: David Hildenbrand <david@xxxxxxxxxx>
>> Closes: https://lore.kernel.org/all/DMHE8BEEER1H.2BIY52NXATPIA@xxxxxxxxxx/
>> Fixes: c010d47f107f6 ("mm: thp: split huge page to any lower order pages")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Acked-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
>> 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].
>>
>> [1] https://lore.kernel.org/all/20261029-data_race_huge_mem-v1-1-112e331291c1@xxxxxxxxx/
>
> Sorry for late reply, I only now got to this.
>
> Setting PG_head atomically on its own looks odd to me. The comment above
> smp_wmb() already says page flags must be visible before the page becomes
> non-compound. prep_compound_page() writing PG_head after
> clear_compound_head() is what breaks that.
>
> Can we build the whole flags word for the new head, PG_head included, and
> store it once before smp_wmb()?

I think it is a better approach.

>
> After clear_compound_head() only the tail and head setup is left, and it
> does not touch new_head->flags. Then SetPageHead() and
> prep_compound_page_atomic() are not needed.
>
> This was option 2 in your analysis[*] on Ye Liu's READ_ONCE() thread,
> but I don't see a barrier problem.

At that time, I did not think about separate __SetPageHead() and
__prep_compound_page() and what I had in mind would still call
prep_compound_page() after clear_compound_head(). That was the source
of the complication.

But your version does not have the issue.

>
> PG_head gets the same ordering as the cloned PG_locked, and
> softleaf_entry_wait_on_locked() already relies on that through the same
> smp_wmb() and page_folio(). The new head carries PG_head for a moment
> while it is still a tail, but page_folio() only looks at compound_info.
>
> Reading the old head's flags with READ_ONCE() while building the word
> would also cover Ye Liu's KCSAN report.

I am planning to use data_race() like I said in Ye Liu’s email, since
the flags are stable at this moment. But let me know if I miss something.

>
> Something like this, on top of the v2 base, compile-tested only:

I will send v3 based on your changes below. Thanks.

>
> [*] https://lore.kernel.org/all/DMHE8BEEER1H.2BIY52NXATPIA@xxxxxxxxxx
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index bd44d58e8684..53786b2abe89 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;
> +		unsigned long flags;
>
>  		VM_BUG_ON_PAGE(atomic_read(&new_folio->_mapcount) != -1, new_head);
>
> @@ -3730,8 +3731,8 @@ 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 = READ_ONCE(new_folio->flags.f) & ~PAGE_FLAGS_CHECK_AT_PREP;
> +		flags |= READ_ONCE(folio->flags.f) &
>  				((1L << PG_referenced) |
>  				 (1L << PG_swapbacked) |
>  				 (1L << PG_swapcache) |
> @@ -3749,7 +3750,16 @@ 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);
> +
> +		/*
> +		 * Set PG_head here, not after clear_compound_head(): from then on
> +		 * softleaf_entry_wait_on_locked() can set PG_waiters on new_head
> +		 * without a reference, and a plain write would lose it.
> +		 */
> +		if (new_order)
> +			flags |= 1L << PG_head;
> +		WRITE_ONCE(new_folio->flags.f, flags);
>
>  		new_folio->mapping = folio->mapping;
>  		new_folio->index = folio->index + i;
> @@ -3779,7 +3789,7 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>  		 */
>  		clear_compound_head(new_head);
>  		if (new_order) {
> -			prep_compound_page(new_head, new_order);
> +			__prep_compound_page(new_head, new_order);
>  			folio_set_large_rmappable(new_folio);
>  		}
>
> diff --git a/mm/internal.h b/mm/internal.h
> index aeebb3e49ed1..4ce67ce2f3fb 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -807,6 +807,8 @@ static inline void prep_compound_tail(struct page *tail,
>  	VM_WARN_ON_ONCE(tail->private);
>  }
>
> +void __prep_compound_page(struct page *page, unsigned int order);
> +
>  #if defined CONFIG_COMPACTION || defined CONFIG_CMA
>
>  /*
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 143776e671ab..37768fa5341b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -683,11 +683,17 @@ static inline bool pcp_allowed_order(unsigned int order)
>   */
>
>  void prep_compound_page(struct page *page, unsigned int order)
> +{
> +	__SetPageHead(page);
> +	__prep_compound_page(page, order);
> +}
> +
> +/* Set up the head and tail pages of a page that already has PG_head */
> +void __prep_compound_page(struct page *page, unsigned int order)
>  {
>  	int i;
>  	int nr_pages = 1 << order;
>
> -	__SetPageHead(page);
>  	for (i = 1; i < nr_pages; i++)
>  		prep_compound_tail(page + i, page, order);
>
> -- 
>   Kiryl Shutsemau / Kirill A. Shutemov


Best Regards,
Yan, Zi