Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mm: test_hmm: deadlock and folio leak on retry in dmirror_range_fault() - [Medium] mm: test_hmm: unmapped device pages on retry in do_fault_and_migrate() - [Low] lib/test_hmm: excessive stack frame in dmirror_fault_and_migrate_to_device() --
commit 7670468c9cdf50d8e222db90061981491fa308c8 Author: Mika Penttilä <[email protected]> lib/test_hmm: add a new testcase for the migrate on fault This commit adds a migrate-on-fault testcase to the HMM test driver and selftests using HMM_PFN_REQ_FAULT | HMM_PFN_REQ_MIGRATE with hmm_range_fault() to fault in and migrate ranges to device-private memory. > diff --git a/lib/test_hmm.c b/lib/test_hmm.c > index f05f801b8404b..2045896493ae3 100644 > --- a/lib/test_hmm.c > +++ b/lib/test_hmm.c > @@ -36,6 +36,7 @@ > #define DMIRROR_RANGE_FAULT_TIMEOUT 1000 > #define DEVMEM_CHUNK_SIZE (256 * 1024 * 1024U) > #define DEVMEM_CHUNKS_RESERVE 16 > +#define PFNS_ARRAY_SIZE 64 > > /* > * For device_private pages, dpage is just a dummy struct page > @@ -355,6 +356,7 @@ static int dmirror_range_fault(struct dmirror *dmirror, > struct mm_struct *mm = dmirror->notifier.mm; > unsigned long timeout = > jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); > + bool migrate = range->default_flags & HMM_PFN_REQ_MIGRATE; > int ret; > > while (true) { > @@ -364,9 +366,15 @@ static int dmirror_range_fault(struct dmirror *dmirror, > } > > range->notifier_seq = mmu_interval_read_begin(range->notifier); > - mmap_read_lock(mm); > - ret = hmm_range_fault(range); > - mmap_read_unlock(mm); > + > + /* mmap lock held for whole migrate on fault operation */ > + if (!migrate) { > + mmap_read_lock(mm); > + ret = hmm_range_fault(range); > + mmap_read_unlock(mm); > + } else { > + ret = hmm_range_fault(range); > + } > if (ret) { > if (ret == -EBUSY) > continue; > @@ -382,7 +390,9 @@ static int dmirror_range_fault(struct dmirror *dmirror, > break; > } > > - ret = dmirror_do_fault(dmirror, range); > + /* update device page table after migration */ > + if (!migrate) > + ret = dmirror_do_fault(dmirror, range); > > mutex_unlock(&dmirror->mutex); > out: [Severity: High] Can retrying inside dmirror_range_fault() when migrate is true cause a self-deadlock and leak folio locks and references? During the first call to hmm_range_fault(), hmm_vma_handle_migrate_prepare() and hmm_vma_handle_migrate_prepare_pmd() collect present pages by locking the folio, taking a folio reference, installing a migration PTE, and setting HMM_PFN_VALID | HMM_PFN_MIGRATE in range->hmm_pfns. If hmm_range_fault() returns -EBUSY, or if hmm_range_fault() returns 0 and mmu_interval_read_retry() trips in dmirror_range_fault() (for example, after migrate_vma_split_folio() splits a PTE-mapped large folio and fires MMU_NOTIFY_CLEAR, or due to a concurrent invalidation), dmirror_range_fault() loops back via continue without returning to do_fault_and_migrate() to abort and finalize the already-collected migration entries. On the next iteration of the loop in dmirror_range_fault(), calling hmm_range_fault() -> hmm_range_fault_locked() zeroes range->hmm_pfns: mm/hmm.c:hmm_range_fault_locked() if (hmm_select_migrate(range)) { WARN_ON_ONCE(range->pfn_flags_mask); WARN_ON_ONCE(range->dev_private_owner != range->migrate->pgmap_owner); memset(range->hmm_pfns, 0, ((range->end - range->start) >> PAGE_SHIFT) * sizeof(*range->hmm_pfns)); } When the retry walk visits the migration PTEs installed by the previous attempt, hmm_pfn_collected(*hmm_pfn) evaluates to false in hmm_vma_handle_pte() because *hmm_pfn was zeroed: mm/hmm.c:hmm_vma_handle_pte() if (softleaf_is_migration(entry) && hmm_select_migrate(range) && hmm_pfn_collected(*hmm_pfn) && hmm_pfn_to_page(*hmm_pfn) == softleaf_to_page(entry)) return 0; It then treats its own migration entry as an external migration entry and calls hmm_do_fault() -> handle_mm_fault() -> migration_entry_wait(), waiting forever on the folio lock already held by current. In addition, if any collected PTEs were unmapped before the retry walk (or if the retry walk fails or times out before revisiting them), zeroing range->hmm_pfns prevents hmm_vma_handle_migrate_prepare_rollback() and migrate_hmm_range_setup() / migrate_vma_finalize() from unlocking those folios and dropping their references. Furthermore, if dmirror_range_fault() times out at time_after(jiffies, timeout) before calling hmm_range_fault() on a subsequent subrange chunk, do_fault_and_migrate() still runs migrate_hmm_range_setup(), migrate_vma_pages(), and migrate_vma_finalize() on the stale src_pfns and migrate->start/end state from the previous chunk. Should dmirror_range_fault() return -EBUSY to do_fault_and_migrate() when migrate is true so the caller can run the migrate_hmm_range_setup(), migrate_vma_pages(), and migrate_vma_finalize() cleanup before retrying? [ ... ] > @@ -1377,6 +1398,127 @@ static int dmirror_migrate_to_device(struct dmirror > *dmirror, > return ret; > } > > +static int do_fault_and_migrate(struct dmirror *dmirror, struct hmm_range > *range) > +{ > + struct migrate_vma *migrate = range->migrate; > + unsigned long timeout = > + jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); > + int ret; > + > + do { > + mmap_read_lock(dmirror->notifier.mm); > + > + /* Fault-in pages for migration */ > + ret = dmirror_range_fault(dmirror, range); > + /* > + * Set this up even on error: hmm_range_fault() may have > collected > + * part of the range before failing. > + */ > + migrate_hmm_range_setup(range); > + if (ret) { > + /* > + * dst[] entries are empty, so this marks every > collected > + * migration as failed. finalize then restores the > source > + * mappings and drops the associated locks/references. > + */ > + migrate_vma_pages(migrate); > + migrate_vma_finalize(migrate); > + goto unlock; > + } > + > + pr_debug("Migrating from sys mem to device mem\n"); > + > + dmirror_migrate_alloc_and_copy(migrate, dmirror); > + migrate_vma_pages(migrate); > + > + mutex_lock(&dmirror->mutex); > + if (mmu_interval_read_retry(&dmirror->notifier, > + range->notifier_seq)) > + ret = -EBUSY; > + else > + ret = __dmirror_migrate_map_locked(migrate, dmirror); > + mutex_unlock(&dmirror->mutex); > + > + migrate_vma_finalize(migrate); > +unlock: > + mmap_read_unlock(dmirror->notifier.mm); > + } while (ret == -EBUSY && !time_after(jiffies, timeout)); > + > + return ret; > +} [Severity: Medium] As reported by Sashiko on v16 [1], when mmu_interval_read_retry() returns true in do_fault_and_migrate() after migrate_vma_pages() has executed, can this leave migrated device-private pages unmapped in dmirror->pt and cause dmirror_fault_and_migrate_to_device() to fail with -ENOENT? When mmu_interval_read_retry() trips here (for example, due to an invalidation anywhere in the process mm since dmirror->notifier covers the entire address space, or a partial zap, or split_huge_pmd() in migrate_vma_insert_page()), __dmirror_migrate_map_locked() is skipped, while migrate_vma_finalize() still commits any non-zapped migration PTEs to device-private memory (and migrate_vma_insert_page() has already installed device-private PTEs for pte_none or zero-page entries). On the subsequent -EBUSY retry iteration of do_fault_and_migrate(), those already-committed device-private pages are reported by hmm_vma_handle_pte() as HMM_PFN_VALID without HMM_PFN_MIGRATE because migrate.flags only sets MIGRATE_VMA_SELECT_SYSTEM, and dmirror_range_fault() skips dmirror_do_fault() because HMM_PFN_REQ_MIGRATE is set. Although the commit message notes that a real device would map committed pages on its next fault, dmirror_fault_and_migrate_to_device() immediately verifies the range by calling dmirror_do_read() directly without a fault retry loop: mutex_lock(&dmirror->mutex); ret = dmirror_do_read(dmirror, start, end, &bounce); mutex_unlock(&dmirror->mutex); Wouldn't dmirror_do_read() find no entry in dmirror->pt for those already-migrated pages and fail the ioctl with -ENOENT? > + > +static int dmirror_fault_and_migrate_to_device(struct dmirror *dmirror, > + struct hmm_dmirror_cmd *cmd) > +{ > + unsigned long start, size, end, next; > + unsigned long src_pfns[PFNS_ARRAY_SIZE] = { 0 }; > + unsigned long dst_pfns[PFNS_ARRAY_SIZE] = { 0 }; > + struct migrate_vma migrate = { 0 }; > + struct hmm_range range = { 0 }; > + struct dmirror_bounce bounce; [Severity: Low] Does allocating src_pfns[64] and dst_pfns[64] on the stack in dmirror_fault_and_migrate_to_device() risk triggering -Wframe-larger-than warnings? On 64-bit architectures, src_pfns and dst_pfns alone consume 1024 bytes of stack space alongside struct migrate_vma, struct hmm_range, and struct dmirror_bounce (exceeding 1230 bytes of local variables before saved registers and inlining do_fault_and_migrate()). Commit 6046a3bed1c2 ("lib/test_hmm: reduce stack usage") previously had to reduce the 64-element stack arrays in dmirror_migrate_to_device() after hitting a 1408-byte stack frame build failure, and commit 775465fd26a3 ("lib/test_hmm: use kvcalloc() for src/dst pfns allocation") moved dmirror_migrate_to_device() and dmirror_migrate_to_system() to dynamic allocation with kvcalloc(). Should dmirror_fault_and_migrate_to_device() also use kvcalloc() or a smaller PFNS_ARRAY_SIZE? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
