Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [Critical] mm: hmm_range_fault_locked() zeroes hmm_pfns on -EBUSY migration retry - [Critical] mm/hmm: missing rollback of earlier entries after fault in hmm_range_fault() - [High] mm: missing folio_test_anon() check in hmm_vma_handle_migrate_prepare_pmd() - [High] mm: early return in hmm_vma_walk_pmd() on folio split failure skips PTEs - [High] mm/hmm: folio_lock deadlock in migrate_vma_split_folio() and hmm_do_fault() - [Medium] mm: lost MIGRATE_PFN_WRITE on writable protnone entries in HMM migration - [Low] mm: build failure on incomplete struct migrate_vma when !CONFIG_MIGRATION - [Low] mm/migrate_device: unused MIGRATE_VMA_FAULT and MIGRATE_VMA_WRITE flags in migrate_vma_setup() --
commit 00843344cdcc1e298091d82f4ddd5b2d3478ddb1 Author: Mika Penttilä <[email protected]> mm: enable device page migration from HMM pagewalk This commit enables migration collection in hmm_range_fault() via hmm_select_migrate(), switches migrate_vma_setup() to use the HMM pagewalk path with new MIGRATE_VMA_FAULT and MIGRATE_VMA_WRITE flags, and removes the old migrate_vma_collect() page table walk helpers. > diff --git a/include/linux/migrate.h b/include/linux/migrate.h > index dd7dd7ec6995e..3780c4c116b8a 100644 > --- a/include/linux/migrate.h > +++ b/include/linux/migrate.h > @@ -181,6 +181,8 @@ enum migrate_vma_info { > MIGRATE_VMA_SELECT_DEVICE_PRIVATE = 1 << 1, > MIGRATE_VMA_SELECT_DEVICE_COHERENT = 1 << 2, > MIGRATE_VMA_SELECT_COMPOUND = 1 << 3, > + MIGRATE_VMA_FAULT = 1 << 4, > + MIGRATE_VMA_WRITE = 1 << 5, > }; [Severity: Low] As reported by Christoph Hellwig on v15 [1], are MIGRATE_VMA_FAULT and MIGRATE_VMA_WRITE in enum migrate_vma_info and their handling in migrate_vma_setup() needed when there are no in-tree callers? Later patches in the series (for test_hmm and drm/nouveau/svm) drive migrate-on-fault directly through hmm_range_fault() with HMM_PFN_REQ_MIGRATE and migrate_hmm_range_setup(), leaving MIGRATE_VMA_FAULT and MIGRATE_VMA_WRITE unused across the tree. > @@ -218,10 +220,14 @@ struct migrate_vma { > struct page *fault_page; > }; > > -// TODO: enable migration > static inline enum migrate_vma_info hmm_select_migrate(struct hmm_range > *range) > { > - return 0; > + enum migrate_vma_info minfo; > + > + minfo = (range->default_flags & HMM_PFN_REQ_MIGRATE) ? > + range->migrate->flags : 0; > + > + return minfo; > } [Severity: Low] Will mm/hmm.c fail to build when CONFIG_HMM_MIRROR=y and CONFIG_MIGRATION=n? Several drivers (such as INFINIBAND_ON_DEMAND_PAGING, DRM_AMDGPU_USERPTR, and MSHV_ROOT) select CONFIG_HMM_MIRROR without selecting CONFIG_MIGRATION. When CONFIG_MIGRATION is disabled, struct migrate_vma is only forward-declared in include/linux/hmm.h, but hmm_range_fault_locked() in mm/hmm.c is compiled outside #ifdef CONFIG_DEVICE_MIGRATION and dereferences range->migrate->pgmap_owner: 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); ... } } In addition, hmm_select_migrate() in include/linux/migrate.h is guarded by #ifdef CONFIG_MIGRATION rather than #ifdef CONFIG_DEVICE_MIGRATION, even though the migration implementation in mm/hmm.c and mm/migrate_device.c depends on CONFIG_DEVICE_MIGRATION. Should hmm_select_migrate() be guarded by CONFIG_DEVICE_MIGRATION (returning 0 when !CONFIG_DEVICE_MIGRATION), and should the range->migrate dereference in hmm_range_fault_locked() be guarded or moved into a helper? [ ... ] > diff --git a/mm/hmm.c b/mm/hmm.c > index 1c8a3fa29c4e6..0b38bf84c3690 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -1658,14 +1658,43 @@ static int hmm_range_fault_locked(struct hmm_range > *range, bool *locked) > * the invalidation to finish. > * -EFAULT: A page was requested to be valid and could not be made valid > * ie it has no backing VMA or it is illegal to access > + * -ERANGE: The range crosses multiple VMAs, or space for hmm_pfns array > + * is too low. > * > * This is similar to get_user_pages(), except that it can read the page > tables > * without mutating them (ie causing faults). > * > * The mmap lock must be held by the caller and will remain held on return. > * New users should prefer hmm_range_fault_unlocked_timeout() unless they > - * specifically need to keep the mmap lock held across the call. This helper > - * cannot support VMAs whose fault handlers need to drop the mmap lock. > + * specifically need to keep the mmap lock held across the call like while > + * migrating. This helper cannot support VMAs whose fault handlers need to > + * drop the mmap lock. > + * > + * If want to do migration after faulting, call hmm_range_fault() with > + * range.default_flags of HMM_PFN_REQ_MIGRATE, and optionally > + * HMM_PFN_REQ_FAULT|HMM_PFN_REQ_WRITE, and initialize range->migrate field. > + * range->migrate->vma will be populated during the call, > + * and must be stable across the whole migrate process, which is > + * why mmap_lock must be held around this call. > + * > + * When HMM_PFN_REQ_MIGRATE is set, migration collection may be partial on > + * return and the caller takes responsibility for completing or aborting it. > + * > + * On success, the caller must call migrate_hmm_range_setup() and may then > + * proceed with the normal migrate_vma sequence: prepare destination pages, > + * call migrate_vma_pages(), update device mappings as needed, and finally > + * call migrate_vma_finalize(). > + * > + * On -EBUSY, the caller may retry hmm_range_fault() using the same range and > + * PFN array without undoing entries collected by the previous attempt. If > + * the caller stops retrying, it must abort the partial migration. [Severity: Critical] When a caller retries hmm_range_fault() after -EBUSY with the same range and PFN array as documented here, won't hmm_range_fault_locked() wipe out all previously collected entries by unconditionally zeroing range->hmm_pfns? At the start of hmm_range_fault_locked() in mm/hmm.c: 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)); } During the first call, hmm_vma_handle_migrate_prepare() and hmm_vma_handle_migrate_prepare_pmd() lock folios, take references, install migration PTEs/PMDs, and record HMM_PFN_VALID | HMM_PFN_MIGRATE in range->hmm_pfns[]. If a later page in the range triggers an mmu_notifier invalidation (for example during hmm_do_fault() or folio splitting) so that mmu_interval_check_retry() returns -EBUSY, retrying hmm_range_fault() zeroes range->hmm_pfns[]. On the retry walk, hmm_pfn_collected() in hmm_vma_handle_pte() and hmm_vma_walk_pmd() evaluates to false for the migration entries installed by the first attempt: 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; } With HMM_PFN_REQ_FAULT set, HMM treats its own migration PTE/PMD as a foreign migration entry and calls hmm_do_fault() -> handle_mm_fault() -> migration_entry_wait(), deadlocking on the folio lock already held by current. Without HMM_PFN_REQ_FAULT (or if the retry aborts before reaching those entries), range->hmm_pfns[] remains zeroed, so migrate_hmm_range_setup() never populates migrate->src[] for those pages and leaks the folio locks, folio references, and migration PTEs. [ ... ] > diff --git a/mm/migrate_device.c b/mm/migrate_device.c > index ba6775a0d37e6..266f9c6d7b607 100644 > --- a/mm/migrate_device.c > +++ b/mm/migrate_device.c [ ... ] > @@ -741,9 +243,20 @@ int migrate_vma_setup(struct migrate_vma *args) > { > const struct vm_area_struct *vma = args->vma; > long nr_pages = (args->end - args->start) >> PAGE_SHIFT; > + struct hmm_range range = { > + .notifier = NULL, > + .hmm_pfns = args->src, > + .dev_private_owner = args->pgmap_owner, > + .migrate = args, > + .default_flags = HMM_PFN_REQ_MIGRATE > + }; > + int ret; > > args->start &= PAGE_MASK; > args->end &= PAGE_MASK; > + range.start = args->start; > + range.end = args->end; > + > if (!vma) > return -EINVAL; > /* The migrate_vma page walk has no hugetlb support. */ > @@ -766,10 +279,24 @@ int migrate_vma_setup(struct migrate_vma *args) > args->cpages = 0; > args->npages = 0; > > - migrate_vma_collect(args); > + if (args->flags & MIGRATE_VMA_FAULT) > + range.default_flags |= HMM_PFN_REQ_FAULT; > > - if (args->cpages) > - migrate_vma_unmap(args); > + if (args->flags & MIGRATE_VMA_WRITE) > + range.default_flags |= HMM_PFN_REQ_FAULT | HMM_PFN_REQ_WRITE; > + > + ret = hmm_range_fault(&range); > + > + migrate_hmm_range_setup(&range); [Severity: Critical] As reported by Sashiko on v16 [2], when hmm_range_fault() faults at an address after already collecting earlier PTEs in the range, can concurrent page table zapping cause a use-after-free of destination pages because the walk resumes at hmm_vma_walk.last without re-validating [range->start, hmm_vma_walk.last)? When HMM_PFN_REQ_MIGRATE | HMM_PFN_REQ_FAULT is used, walk_page_range() collects present pages in [range->start, addr) via hmm_vma_handle_migrate_prepare(), installing migration PTEs and setting HMM_PFN_VALID | HMM_PFN_MIGRATE in range->hmm_pfns[]. When a later address addr requires a fault, hmm_vma_handle_pte() drops the PTE lock, sets hmm_vma_walk.last = addr via hmm_record_fault(), and returns HMM_FAULT_PENDING: mm/hmm.c:hmm_range_fault_locked() { ret = walk_page_range(mm, hmm_vma_walk.last, range->end, &hmm_walk_ops, &hmm_vma_walk); if (ret == HMM_FAULT_PENDING) { hmm_vma_migrate_disarm(&hmm_vma_walk); ret = hmm_do_fault(mm, &hmm_vma_walk); ... } } While the PTE spinlock and migration mmu_notifier invalidation are dropped during hmm_do_fault(), a concurrent madvise(MADV_DONTNEED) only requires mmap_read_lock and can clear the migration PTEs in [range->start, addr) via zap_pte_range() -> zap_nonpresent_ptes() without taking the folio lock. When the loop resumes walk_page_range() at hmm_vma_walk.last = addr, hmm_vma_handle_migrate_prepare_rollback() only inspects [addr, end) and never re-checks [range->start, addr). Those zapped entries keep HMM_PFN_VALID | HMM_PFN_MIGRATE in range->hmm_pfns[] and pass migrate_vma_check_page() because folio_mapped(src) is false. Later, in __migrate_device_finalize(), remove_migration_ptes(src, dst, 0) finds no migration PTEs to replace (so dst is never mapped into the CPU page tables or given a page table refcount), and folio_put(dst) frees the destination folio while the device driver still has it mapped. [Severity: High] As reported by Sashiko on v16 [3], can switching migrate_vma_setup() and hmm_range_fault() to the HMM migration path deadlock on folio locks in migrate_vma_split_folio() and hmm_do_fault(), or cause spurious split_folio() -EAGAIN failures? First, during migration collection, hmm_vma_handle_migrate_prepare() keeps all previously collected folios in the range locked (using folio_trylock() to avoid deadlocks). However, when it encounters a PTE-mapped large folio, it calls migrate_vma_split_folio(), which takes a reference and does a blocking folio_lock() while still holding the locks and migration PTEs on fault_folio and all earlier collected folios: mm/hmm.c:migrate_vma_split_folio() { if (folio != fault_folio) folio_get(folio); lazy_mmu_mode_disable(); pte_unmap_unlock(ptep, hmm_vma_walk->ptl); hmm_vma_walk->ptelocked = false; if (folio != fault_folio) folio_lock(folio); return __migrate_vma_split_folio(folio, fault_page); } If another task holds folio_lock(folio) and waits on one of the earlier migration PTEs via migration_entry_wait() (or tries to lock one of the already-collected folios), both tasks deadlock. Taking folio_get(folio) before folio_lock(folio) also means a concurrent caller blocking on folio_lock(folio) elevates the refcount and causes the lock holder's split_folio() to fail with -EAGAIN. Second, when HMM_PFN_REQ_FAULT is used with HMM_PFN_REQ_MIGRATE, encountering another thread's migration entry in hmm_vma_handle_pte() or hmm_vma_walk_pmd() (or a swap or device-private entry) invokes hmm_do_fault() -> handle_mm_fault(), which sleeps in migration_entry_wait() or folio_lock() while still holding the locks and migration PTEs on all folios collected earlier in the walk. [Severity: High] Does hmm_vma_handle_migrate_prepare_pmd() in mm/hmm.c miss a folio_test_anon(folio) check when collecting or splitting a PMD-mapped THP? At the PTE level, hmm_vma_handle_migrate_prepare() rejects non-anonymous folios: mm/hmm.c:hmm_vma_handle_migrate_prepare() { /* Only anonymous memory can be migrated to device memory */ if (!page || !folio_test_anon(page_folio(page))) goto out; } However, hmm_vma_handle_migrate_prepare_pmd() selects any non-zero pmd_trans_huge(*pmdp) folio when MIGRATE_VMA_SELECT_SYSTEM is set: mm/hmm.c:hmm_vma_handle_migrate_prepare_pmd() { if (pmd_trans_huge(*pmdp)) { if (!(minfo & MIGRATE_VMA_SELECT_SYSTEM)) goto out; folio = pmd_folio(*pmdp); if (is_huge_zero_folio(folio)) return hmm_pfns_fill(start, end, hmm_vma_walk, 0); } } If HMM migration runs on a private file-backed or shmem VMA containing a PMD-mapped page-cache/shmem THP, hmm_vma_handle_migrate_prepare_pmd() locks the non-anonymous folio and either installs a PMD migration entry (holding the file/shmem folio lock across subsequent faults in hmm_do_fault(), which take mapping->invalidate_lock and invert the mapping->invalidate_lock -> folio_lock ordering) or splits the THP even though hmm_vma_handle_migrate_prepare() will reject all of its PTEs. [Severity: High] In hmm_vma_walk_pmd() in mm/hmm.c, when splitting a PTE-mapped large folio fails in hmm_vma_handle_migrate_prepare(), doesn't returning 0 immediately skip the remaining PTEs in [addr + PAGE_SIZE, end)? In the PTE loop of hmm_vma_walk_pmd(): mm/hmm.c:hmm_vma_walk_pmd() { for (; addr < end; addr += PAGE_SIZE, ptep++, hmm_pfns++) { ... r = hmm_vma_handle_pte(walk, addr, end, pmdp, ptep, hmm_pfns); ... r = hmm_vma_handle_migrate_prepare(walk, pmdp, addr, ptep, hmm_pfns, &unmapped); if (r == -EAGAIN) { addr = start; goto again; } if (r) { HMM_ASSERT_UNLOCKED(hmm_vma_walk); if (unmapped) flush_tlb_range(walk->vma, start, addr); return 0; } } } When migrate_vma_split_folio() fails at addr, it drops the PTE lock and hmm_vma_handle_migrate_prepare() returns -EFAULT. Returning 0 from hmm_vma_walk_pmd() tells walk_page_range() that the entire PMD range [start, end) is complete, leaving [addr + PAGE_SIZE, end) unvisited and unpopulated in range->hmm_pfns[]. Moreover, if an earlier pass in hmm_vma_walk_pmd() had already collected PTEs at addresses above addr before restarting via addr = start; goto again, dropping the PTE lock at addr and returning 0 skips re-verifying and rolling back those collected entries in [addr + PAGE_SIZE, end). [Severity: Medium] Can writable protnone PTEs and PMDs lose MIGRATE_PFN_WRITE in migrate->src[] when migrated via migrate_vma_setup() or hmm_range_fault() without HMM_PFN_REQ_FAULT? For a present protnone PTE or PMD, pte_to_hmm_pfn_flags() and pmd_to_hmm_pfn_flags() return 0, so HMM_PFN_WRITE is not set in *hmm_pfn. Then hmm_vma_handle_migrate_prepare() and hmm_vma_handle_migrate_prepare_pmd() collect the protnone entry, install a writable migration entry if pte_write(pte) or pmd_write(*pmdp) is true, and set HMM_PFN_VALID | HMM_PFN_MIGRATE in *hmm_pfn, but never set HMM_PFN_WRITE: mm/hmm.c:hmm_vma_handle_migrate_prepare() { set_pte_at(mm, addr, ptep, swp_pte); folio_remove_rmap_pte(folio, page, walk->vma); folio_put(folio); *hmm_pfn |= HMM_PFN_VALID | HMM_PFN_MIGRATE; } When migrate_hmm_range_setup() builds migrate->src[i] from range->hmm_pfns[i]: mm/migrate_device.c:migrate_hmm_range_setup() { migrate->src[i] |= MIGRATE_PFN_MIGRATE; migrate->src[i] |= (pfn & HMM_PFN_WRITE) ? MIGRATE_PFN_WRITE : 0; migrate->src[i] |= (pfn & HMM_PFN_COMPOUND) ? MIGRATE_PFN_COMPOUND : 0; } MIGRATE_PFN_WRITE is left unset even though the deleted migrate_vma_collect_pmd() and migrate_vma_collect_huge_pmd() helpers preserved MIGRATE_PFN_WRITE whenever pte_write(pte) or pmd_write(*pmdp) was set. Should hmm_vma_handle_migrate_prepare() and hmm_vma_handle_migrate_prepare_pmd() also set HMM_PFN_WRITE on *hmm_pfn when writable is true? [1] https://lore.kernel.org/r/[email protected] [2] https://lore.kernel.org/r/[email protected] [3] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
