Re: [PATCH 05/13] mm/migrate_device: don't collect a range twice

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mm/migrate: livelock in migrate_vma_split_folio() [1]
--

commit d718421e16d638aec3608447abd867a1a5760540
Author: Josef Bacik <josef@toxicpanda.com>

mm/migrate_device: don't collect a range twice

This commit passes addr instead of start to migrate_vma_collect_huge_pmd()
and flushes the TLB when migrate_vma_collect_pmd() restarts at the "again"
label after splitting a large folio, avoiding collecting [start, addr) a
second time.

> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index edcc13edbf6af..4d4054fd020ea 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -257,13 +257,17 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>  	pte_t *ptep;
>  
>  again:
> +	/* [start, addr) is collected if we get here after splitting a folio */
>  	if (pmd_trans_huge(*pmdp) || !pmd_present(*pmdp)) {
> -		int ret = migrate_vma_collect_huge_pmd(pmdp, start, end, walk, fault_folio);
> +		int ret = migrate_vma_collect_huge_pmd(pmdp, addr, end, walk, fault_folio);
>  
>  		if (ret == -EAGAIN)
>  			goto again;
> -		if (ret == 0)
> +		if (ret == 0) {
> +			if (unmapped)
> +				flush_tlb_range(walk->vma, start, end);
>  			return 0;
> +		}
>  	}
>  
>  	ptep = pte_offset_map_lock(mm, pmdp, start, &ptl);

[Severity: High]
This problem wasn't introduced by this patch, but can concurrent threads
splitting the same large folio livelock in migrate_vma_split_folio()
[1]?

When migrate_vma_collect_pmd() encounters a large folio, it takes a folio
reference while holding the page table lock, drops the page table lock, and
then locks the folio in migrate_vma_split_folio():

mm/migrate_device.c:migrate_vma_collect_pmd() {
    ...
			if (folio_test_large(folio)) {
				int ret;

				/* migrate_vma_split_folio() consumes this reference */
				if (folio != fault_folio)
					folio_get(folio);
				lazy_mmu_mode_disable();
				pte_unmap_unlock(ptep, ptl);
				ret = migrate_vma_split_folio(folio,
							  migrate->fault_page);
    ...
}

If two threads concurrently collect or fault on the same large folio, both
increment the folio reference count before waiting on folio_lock():

CPU0                                CPU1
migrate_vma_collect_pmd()
  folio_get(folio)
  pte_unmap_unlock(ptep, ptl)
                                    migrate_vma_collect_pmd()
                                      folio_get(folio)
                                      pte_unmap_unlock(ptep, ptl)
  migrate_vma_split_folio()
    folio_lock(folio)
    split_folio(folio)  <-- fails (-EAGAIN) due to CPU1's extra ref
    folio_unlock(folio)
    folio_put(folio)
                                      migrate_vma_split_folio()
                                        folio_lock(folio)
                                        split_folio(folio)  <-- fails

Because each waiting thread holds an extra reference on the folio,
split_folio() sees unexpected references and fails with -EAGAIN, causing
faulting threads to repeatedly retry and livelock.

[1] https://sashiko.dev/#/bug/linux-466ab9fe-179c-44b5-9682-18591b13f65d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-mason-migrate-et-al-fixes-v1-0-3d2454362e8d@kernel.org?part=5