Re: [PATCH] mm/migrate: fix stale partially_mapped arg to deferred_split_folio()

[David Hildenbrand (Arm)]

On 4/1/26 10:41, Deepanshu Kartikey wrote:
> In migrate_folio_move(), src_partially_mapped is sampled from the source
> folio before move_to_new_folio() is called:
> 
>   if (folio_order(src) > 1 &&
>       !data_race(list_empty(&src->_deferred_list))) {
>           src_deferred_split = true;
>           src_partially_mapped = folio_test_partially_mapped(src);
>   }
> 
> A concurrent thread can unmap pages from the source folio between this
> read and the actual migration, making the sampled value stale.

Indeed. We could see "partially_mapped" getting set concurrently.

> 
> After move_to_new_folio() succeeds, __folio_migrate_mapping() has already
> copied all folio flags including PG_partially_mapped from src to dst.
> Passing the stale src_partially_mapped=false to deferred_split_folio(dst)
> while dst already has PG_partially_mapped=true triggers the invariant
> check in deferred_split_folio():
> 
>   VM_WARN_ON_FOLIO(folio_test_partially_mapped(folio), folio)
> 
> at mm/huge_memory.c:4371, because the argument contradicts the flag
> already set on the folio.
> 
> Fix this by removing the src_partially_mapped variable entirely and
> reading PG_partially_mapped directly from dst after move_to_new_folio()
> completes, where it is authoritative and race-free.
> 
> Fixes: 8a8ca142a488 ("mm: migrate: requeue destination folio on deferred split queue")
> Reported-by: syzbot+a7067a757858ac8eb085@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=a7067a757858ac8eb085
> Tested-by: syzbot+a7067a757858ac8eb085@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> ---
>  mm/migrate.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 05cb408846f2..11236779e910 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1361,7 +1361,6 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
>  	int old_page_state = 0;
>  	struct anon_vma *anon_vma = NULL;
>  	bool src_deferred_split = false;
> -	bool src_partially_mapped = false;
>  	struct list_head *prev;
>  
>  	__migrate_folio_extract(dst, &old_page_state, &anon_vma);
> @@ -1378,7 +1377,6 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
>  	if (folio_order(src) > 1 &&
>  	    !data_race(list_empty(&src->_deferred_list))) {
>  		src_deferred_split = true;
> -		src_partially_mapped = folio_test_partially_mapped(src);
>  	}
>  
>  	rc = move_to_new_folio(dst, src, mode);
> @@ -1404,11 +1402,13 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
>  	/*
>  	 * Requeue the destination folio on the deferred split queue if
>  	 * the source was on the queue.  The source is unqueued in
> -	 * __folio_migrate_mapping(), so we recorded the state from
> -	 * before move_to_new_folio().
> +	 * __folio_migrate_mapping(). Read PG_partially_mapped directly from
> +	 * dst: move_to_new_folio() copies all flags from src to dst, so dst
> +	 * now holds the correct authoritative state.

Where are we copying PG_partially_mapped exactly?

I don't see that happening in folio_migrate_flags().


Also, I wonder what happens if the folio gets partially mapped after
already installing migration entries (zapping migration entries).

This is all extremely shaky.

-- 
Cheers,

David