[PATCH] mm/migrate_device: fix double unlock and remove dead code

[PATCH] mm/migrate_device: fix double unlock and remove dead code

[Sunny Patel]

Fix two bugs in device migration paths:

1) migrate_vma_collect_huge_pmd() calls spin_unlock after
   softleaf_entry_wait_on_locked(), which already releases the ptl.

2) migrate_vma_insert_huge_pmd_page() has a dead else-if branch and this
   branch is always unreachable.

Signed-off-by: Sunny Patel <nueralspacetech@gmail.com>
---
 mm/migrate_device.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index 8079676c8f1f..0e005c26ee88 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -177,7 +177,6 @@ static int migrate_vma_collect_huge_pmd(pmd_t *pmdp, unsigned long start,
 
 		if (softleaf_is_migration(entry)) {
 			softleaf_entry_wait_on_locked(entry, ptl);
-			spin_unlock(ptl);
 			return -EAGAIN;
 		}
 
@@ -869,8 +868,7 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
 		if (!is_huge_zero_pmd(*pmdp))
 			goto unlock_abort;
 		flush = true;
-	} else if (!pmd_none(*pmdp))
-		goto unlock_abort;
+	}
 
 	add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
 	folio_add_new_anon_rmap(folio, vma, addr, RMAP_EXCLUSIVE);
-- 
2.43.0

Re: [PATCH] mm/migrate_device: fix double unlock and remove dead code

[David Hildenbrand (Arm)]

On 4/13/26 15:09, Sunny Patel wrote:
> Fix two bugs in device migration paths:
> 
> 1) migrate_vma_collect_huge_pmd() calls spin_unlock after
>    softleaf_entry_wait_on_locked(), which already releases the ptl.
> 
> 2) migrate_vma_insert_huge_pmd_page() has a dead else-if branch and this
>    branch is always unreachable.
> 

Can you move 1) into a separate patch, add a Fixes: tag an CC stable?

I think it is

Fixes: a30b48bf1b24 ("mm/migrate_device: implement THP migration of zone
device pages")

2) will then be a pure cleanup patch.

Thanks!

> Signed-off-by: Sunny Patel <nueralspacetech@gmail.com>
> ---
>  mm/migrate_device.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 8079676c8f1f..0e005c26ee88 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -177,7 +177,6 @@ static int migrate_vma_collect_huge_pmd(pmd_t *pmdp, unsigned long start,
>  
>  		if (softleaf_is_migration(entry)) {
>  			softleaf_entry_wait_on_locked(entry, ptl);
> -			spin_unlock(ptl);


Yes, that looks correct to me.

>  			return -EAGAIN;
>  		}
>  
> @@ -869,8 +868,7 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>  		if (!is_huge_zero_pmd(*pmdp))
>  			goto unlock_abort;
>  		flush = true;
> -	} else if (!pmd_none(*pmdp))
> -		goto unlock_abort;
> +	}

Huh, how did that happen. I hope that it's not a typo and we wanted to
check for something else.

-- 
Cheers,

David

Re: [PATCH] mm/migrate_device: fix double unlock and remove dead code

[Zi Yan]

On 13 Apr 2026, at 15:38, David Hildenbrand (Arm) wrote:

> On 4/13/26 15:09, Sunny Patel wrote:
>> Fix two bugs in device migration paths:
>>
>> 1) migrate_vma_collect_huge_pmd() calls spin_unlock after
>>    softleaf_entry_wait_on_locked(), which already releases the ptl.
>>
>> 2) migrate_vma_insert_huge_pmd_page() has a dead else-if branch and this
>>    branch is always unreachable.
>>
>
> Can you move 1) into a separate patch, add a Fixes: tag an CC stable?
>
> I think it is
>
> Fixes: a30b48bf1b24 ("mm/migrate_device: implement THP migration of zone
> device pages")
>
> 2) will then be a pure cleanup patch.
>
> Thanks!
>
>> Signed-off-by: Sunny Patel <nueralspacetech@gmail.com>
>> ---
>>  mm/migrate_device.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>> index 8079676c8f1f..0e005c26ee88 100644
>> --- a/mm/migrate_device.c
>> +++ b/mm/migrate_device.c
>> @@ -177,7 +177,6 @@ static int migrate_vma_collect_huge_pmd(pmd_t *pmdp, unsigned long start,
>>
>>  		if (softleaf_is_migration(entry)) {
>>  			softleaf_entry_wait_on_locked(entry, ptl);
>> -			spin_unlock(ptl);
>
>
> Yes, that looks correct to me.
>
>>  			return -EAGAIN;
>>  		}
>>
>> @@ -869,8 +868,7 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>>  		if (!is_huge_zero_pmd(*pmdp))
>>  			goto unlock_abort;
>>  		flush = true;
>> -	} else if (!pmd_none(*pmdp))
>> -		goto unlock_abort;
>> +	}
>
> Huh, how did that happen. I hope that it's not a typo and we wanted to
> check for something else.
>


Looking at the function and trying to figure this out, but find
VM_WARN_ON_FOLIO(!folio, folio) at the top, where folio is from page_folio(page).
It is either a nop or a zero dereferencing. It should be removed.

VM_WARN_ON_ONCE(!pmd_none(*pmdp) && !is_huge_zero_pmd(*pmdp)) can be removed
too, since the above ifs takes !pmd_none(*pmdp) && !is_huge_zero_pmd(*pmdp)
to unlock_abort.

Back to the above code:

        if (!pmd_none(*pmdp)) {
                if (!is_huge_zero_pmd(*pmdp))
                        goto unlock_abort;
                flush = true;
        } else if (!pmd_none(*pmdp))
                goto unlock_abort;

It seems to me that the first if should be removed, since if pmdp is
not pmd_none(), others filled pmd entry before us, so no further
action should be taken, otherwise, the function will overwrite
some valid pmd entry.

OK, look at migrate_vma_insert_page(), which does PTE level work,
the above code might be intended to do:

        if (pmd_present(*pmdp)) {
                if (!is_huge_zero_pmd(*pmdp))
                        goto unlock_abort;
                flush = true;
        } else if (!pmd_none(*pmdp))
                goto unlock_abort;



Best Regards,
Yan, Zi