Re: [PATCH 1/2] migrate: Correct lock ordering for hugetlb file folios

["David Hildenbrand (Red Hat)" <david@kernel.org>]

On 1/9/26 05:13, Matthew Wilcox (Oracle) wrote:
> Syzbot has found a deadlock (analyzed by Lance Yang):
> 
> 1) Task (5749): Holds folio_lock, then tries to acquire i_mmap_rwsem(read lock).
> 2) Task (5754): Holds i_mmap_rwsem(write lock), then tries to acquire
> folio_lock.
> 
> migrate_pages()
>    -> migrate_hugetlbs()
>      -> unmap_and_move_huge_page()     <- Takes folio_lock!
>        -> remove_migration_ptes()
>          -> __rmap_walk_file()
>            -> i_mmap_lock_read()       <- Waits for i_mmap_rwsem(read lock)!
> 
> hugetlbfs_fallocate()
>    -> hugetlbfs_punch_hole()           <- Takes i_mmap_rwsem(write lock)!
>      -> hugetlbfs_zero_partial_page()
>       -> filemap_lock_hugetlb_folio()
>        -> filemap_lock_folio()
>          -> __filemap_get_folio        <- Waits for folio_lock!


As raised in the other patch I stumbled over first:

We now handle file-backed folios correctly I think. Could we somehow
also be in trouble for anon folios? Because there, we'd still take the
rmap lock after grabbing the folio lock.


> 
> The migration path is the one taking locks in the wrong order according
> to the documentation at the top of mm/rmap.c.  So expand the scope of the
> existing i_mmap_lock to cover the calls to remove_migration_ptes() too.
> 
> This is (mostly) how it used to be after commit c0d0381ade79.  That was
> removed by 336bf30eb765 for both file & anon hugetlb pages when it should
> only have been removed for anon hugetlb pages.
> 
> Fixes: 336bf30eb765 (hugetlbfs: fix anon huge page migration race)
> Reported-by: syzbot+2d9c96466c978346b55f@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/all/68e9715a.050a0220.1186a4.000d.GAE@google.com
> Debugged-by: Lance Yang <lance.yang@linux.dev>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Cc: stable@vger.kernel.org
> ---
>   mm/migrate.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5169f9717f60..4688b9e38cd2 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1458,6 +1458,7 @@ static int unmap_and_move_huge_page(new_folio_t get_new_folio,
>   	int page_was_mapped = 0;
>   	struct anon_vma *anon_vma = NULL;
>   	struct address_space *mapping = NULL;
> +	enum ttu_flags ttu = 0;
>   
>   	if (folio_ref_count(src) == 1) {
>   		/* page was freed from under us. So we are done. */
> @@ -1498,8 +1499,6 @@ static int unmap_and_move_huge_page(new_folio_t get_new_folio,
>   		goto put_anon;
>   
>   	if (folio_mapped(src)) {
> -		enum ttu_flags ttu = 0;
> -
>   		if (!folio_test_anon(src)) {
>   			/*
>   			 * In shared mappings, try_to_unmap could potentially
> @@ -1516,16 +1515,17 @@ static int unmap_and_move_huge_page(new_folio_t get_new_folio,
>   
>   		try_to_migrate(src, ttu);
>   		page_was_mapped = 1;
> -
> -		if (ttu & TTU_RMAP_LOCKED)
> -			i_mmap_unlock_write(mapping);
>   	}
>   
>   	if (!folio_mapped(src))
>   		rc = move_to_new_folio(dst, src, mode);
>   
>   	if (page_was_mapped)
> -		remove_migration_ptes(src, !rc ? dst : src, 0);
> +		remove_migration_ptes(src, !rc ? dst : src,
> +				ttu ? RMP_LOCKED : 0);

	(ttu & TTU_RMAP_LOCKED) ? RMP_LOCKED : 0)

Would be cleaner, but I see how you clean that up in #2. :)

Acked-by: David Hildenbrand (Red Hat) <david@kernel.org>

-- 
Cheers

David