Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [Critical] mm: hmm_vma_handle_pte() corrupts migration PTE state during migration - [High] mm: migrate_vma_setup() skips non-VM_READ VMAs via hmm_vma_walk_test() - [High] mm: hmm_vma_handle_pte() faults entire PMD range [addr, end) on fault - [High] mm: hmm_range_fault_locked() calls handle_mm_fault() under folio_lock() - [High] mm: hmm_vma_walk_pmd() fails to roll back collected entries on retry - [High] mm: uninitialized migrate bounds and pfn offset in hmm_range_fault() - [High] mm: self-invalidation of range->notifier during hmm_range_fault() - [High] mm/hmm: refcount-before-lock and blocking folio_lock() in migrate_vma_split_folio() - [Medium] mm: migrate_vma_setup() skips protnone pages when fault flag is unset --
commit 6029fe80390f5816cec1bea4014e1df1b5d1785d Author: Mika Penttilä <[email protected]> mm: enable device page migration from HMM pagewalk This commit enables device page migration through the HMM page table walk in hmm_range_fault(), switches migrate_vma_setup() to use hmm_range_fault() and migrate_hmm_range_setup(), and removes the legacy migrate_vma_collect() walk. > 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, > }; > > struct migrate_vma { > @@ -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: Critical] When hmm_select_migrate() is enabled, can hmm_vma_handle_pte() in mm/hmm.c roll back an already-installed migration PTE without restoring the page table entry, and also skip waiting on foreign migration entries? In mm/hmm.c:hmm_vma_handle_pte(): if (!pte_present(pte)) { const softleaf_t entry = softleaf_from_pte(pte); ... required_fault = hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0); if (!required_fault) goto out; ... if (softleaf_is_migration(entry)) { if (!hmm_select_migrate(range)) { HMM_ASSERT_UNLOCKED(hmm_vma_walk); pte_unmap(ptep); hmm_vma_walk->last = addr; migration_entry_wait(walk->mm, pmdp, addr); return -EBUSY; } return 0; } ... out: hmm_vma_handle_migrate_prepare_rollback(hmm_vma_walk, addr, addr + PAGE_SIZE, hmm_pfn); *hmm_pfn = (*hmm_pfn & HMM_PFN_INOUT_FLAGS) | new_pfn_flags; return 0; First, when HMM_PFN_REQ_FAULT is not set (the default in migrate_vma_setup()), required_fault is 0. If hmm_vma_handle_migrate_prepare() collects a page at addr (locking the folio, taking a reference, installing a migration PTE, and setting HMM_PFN_VALID | HMM_PFN_MIGRATE in *hmm_pfn) and that PTE is visited again (for example when hmm_vma_walk_pmd() restarts from start after !ptep, when hmm_range_fault_locked() loops on -EBUSY after a THP split, or when a caller retries hmm_range_fault() on -EBUSY), hmm_vma_handle_pte() sees !pte_present(pte) and jumps to out: before checking softleaf_is_migration(). At out:, hmm_vma_handle_migrate_prepare_rollback() unlocks the folio, drops its reference, and clears *hmm_pfn to 0, leaving the migration PTE dangling in the page table. A similar issue happens if migrate_vma_split_folio() fails in hmm_vma_walk_pmd() and calls hmm_pfns_fill(addr, end, hmm_vma_walk, HMM_PFN_ERROR) over entries that were already collected in a prior pass. Second, when HMM_PFN_REQ_FAULT is set and hmm_vma_handle_pte() or hmm_vma_walk_pmd() encounters a migration PTE or PMD installed by another thread (!hmm_pfn_collected(*hmm_pfn)), checking if (!hmm_select_migrate(range)) or if (!minfo) causes the walk to return 0 without waiting via migration_entry_wait() / pmd_migration_entry_wait() and without populating *hmm_pfn. Should hmm_vma_handle_pte() check softleaf_is_migration(entry) before the !required_fault branch and distinguish entries already collected by this walk (hmm_pfn_collected(*hmm_pfn)) from foreign migration entries? [Severity: High] Also in mm/hmm.c:hmm_vma_walk_pmd(), when minfo is non-zero and the PTE lock is dropped mid-PMD (such as in migrate_vma_split_folio() returning -EAGAIN at addr > start), could already-collected entries in [start, addr) fail to be rolled back if the PTE page table disappears or is replaced? In mm/hmm.c:hmm_vma_walk_pmd(): if (minfo) { ptep = pte_offset_map_lock(mm, pmdp, addr, &hmm_vma_walk->ptl); if (ptep) { lazy_mmu_mode_enable(); hmm_vma_walk->ptelocked = true; } else { /* The pte table is gone */ hmm_vma_handle_migrate_prepare_rollback(walk->private, addr, end, hmm_pfns); } } else { ptep = pte_offset_map(pmdp, addr); } if (!ptep) { addr = start; goto again; } When addr > start, the entries collected earlier in the same PMD are in [start, addr) starting at hmm_pfns_start, whereas [addr, end) starting at hmm_pfns is the unwalked remainder of the PMD. Passing addr, end, hmm_pfns to hmm_vma_handle_migrate_prepare_rollback() leaves [start, addr) locked and referenced with HMM_PFN_VALID | HMM_PFN_MIGRATE set. Moreover, if addr is reset to start and goto again finds that *pmdp is now a PMD migration entry (pmd_is_migration_entry(pmd)), or if hmm_vma_walk_hole(), hmm_vma_handle_pmd(), or hmm_vma_handle_absent_pmd() returns HMM_FAULT_PENDING or -EFAULT before reaching their rollback calls, the previously collected folios in hmm_pfns_start are never rolled back either. Should the rollback when !ptep (and before early returns after goto again) cover start to end using hmm_pfns_start? [Severity: High] Enabling hmm_select_migrate() also activates migrate_vma_split_folio() in mm/hmm.c when hmm_vma_handle_migrate_prepare() encounters a PTE-mapped large folio: static int migrate_vma_split_folio(struct folio *folio, struct page *fault_page, struct hmm_vma_walk *hmm_vma_walk, pte_t *ptep) { struct folio *fault_folio = fault_page ? page_folio(fault_page) : NULL; 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); } Unlike hmm_vma_handle_migrate_prepare_pmd() and the order-0 path in hmm_vma_handle_migrate_prepare(), which use folio_trylock() because current may already hold folio_lock on fault_folio and on folios collected at earlier addresses in the walk, migrate_vma_split_folio() calls blocking folio_lock(). Can this deadlock if two concurrent migrations lock folios in opposite order, or if another task holds folio_lock(folio) while waiting on a migration entry installed earlier in this walk? In addition, calling folio_get(folio) before blocking on folio_lock(folio) means that if two tasks concurrently enter migrate_vma_split_folio() on the same large folio, the waiting task's extra reference will cause the lock holder's split_folio() call to fail with -EAGAIN. Would it be safer to use folio_trylock() here as well? > diff --git a/mm/hmm.c b/mm/hmm.c > index 5a1471fd5d288..4d853ee8e8b8c 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -1542,14 +1542,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. > + * > + * On any other error, or when abandoning a retry, the caller must call > + * migrate_hmm_range_setup(), migrate_vma_pages() with no valid destination > + * entries, and migrate_vma_finalize() to abort the partial migration. [Severity: High] What happens in migrate_hmm_range_setup() if hmm_range_fault() fails before hmm_vma_capture_migrate_range() arms hmm_vma_walk->mmu_armed, or if range->start begins in an unmapped hole before walk->vma->vm_start? In mm/hmm.c:hmm_vma_post_range_fault(): if (hmm_select_migrate(range) && hmm_vma_walk->mmu_armed) { if (range->notifier) { hmm_fill_migrate_vma(range, hmm_vma_walk->vma, hmm_vma_walk->start, hmm_vma_walk->end); } mmu_notifier_invalidate_range_end(&hmm_vma_walk->mmu_range); } When a caller drives migration directly through hmm_range_fault() with range->notifier set, range->migrate->vma, range->migrate->start, and range->migrate->end are only populated if hmm_vma_walk->mmu_armed is true. If hmm_range_fault_locked() fails before mmu_armed is set (for instance, if mmu_interval_check_retry() returns -EBUSY on entry, hmm_vma_walk_hole() returns -EFAULT when !walk->vma, or hmm_vma_capture_migrate_range() returns -ERANGE), calling migrate_hmm_range_setup() as documented above will compute migrate->npages = (migrate->end - migrate->start) >> PAGE_SHIFT from uninitialized or stale fields in range->migrate. Additionally, in hmm_vma_capture_migrate_range(): if (end - start > range->end - range->start) return -ERANGE; if (!hmm_vma_walk->mmu_armed) { hmm_vma_walk->start = start; hmm_vma_walk->end = end; Because walk_page_range() clamps start and end to the VMA boundaries within [range->start, range->end), end - start is never greater than range->end - range->start. And if range->start < vma->vm_start (with HMM_PFN_REQ_FAULT unset across the hole), hmm_vma_walk->start is set to vma->vm_start while hmm_vma_walk_pmd() indexes range->hmm_pfns by (addr - range->start) >> PAGE_SHIFT. When migrate_hmm_range_setup() then reads range->hmm_pfns[0 .. migrate->npages - 1], won't it read from the beginning of range->hmm_pfns instead of the VMA offset, misaligning migrate->src[] and leaking any collected folios and migration PTEs at the end of the range? [Severity: High] When hmm_range_fault() is called with both range->notifier and HMM_PFN_REQ_MIGRATE set, won't internal MMU notifier invalidations during the walk trip mmu_interval_check_retry() and cause premature termination of the internal -EBUSY loop? In mm/hmm.c:hmm_range_fault_locked(): do { /* If range is no longer valid force retry. */ if (range->notifier && mmu_interval_check_retry(range->notifier, range->notifier_seq)) { ret = -EBUSY; break; } ret = walk_page_range(mm, hmm_vma_walk.last, range->end, &hmm_walk_ops, &hmm_vma_walk); if (ret == HMM_FAULT_PENDING) { ret = hmm_do_fault(mm, &hmm_vma_walk); ... } } while (ret == -EBUSY); hmm_vma_post_range_fault(&hmm_vma_walk); return ret; During the walk, hmm_vma_capture_migrate_range() calls mmu_notifier_invalidate_range_start() with MMU_NOTIFY_MIGRATE, and any page fault in hmm_do_fault() (such as a CoW or swap fault) or THP split in hmm_vma_handle_migrate_prepare_pmd() triggers an MMU notifier invalidation with owner = NULL. If range->notifier covers the migrated range, those invalidations increment range->notifier's sequence count. When hmm_do_fault() or hmm_vma_handle_migrate_prepare_pmd() returns -EBUSY to re-walk the faulted or split pages, mmu_interval_check_retry() will immediately break out of the loop with -EBUSY, and hmm_vma_post_range_fault() will call mmu_notifier_invalidate_range_end() while partial migration PTEs from earlier addresses are still installed. > + * > + * The mmap read lock must remain held until the migration has either been > + * completed or aborted. > */ > int hmm_range_fault(struct hmm_range *range) > { > diff --git a/mm/migrate_device.c b/mm/migrate_device.c > index 3bcb398c4e02c..c61a9e54e06bb 100644 > --- a/mm/migrate_device.c > +++ b/mm/migrate_device.c [ ... ] > @@ -764,10 +276,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; [Severity: High] When MIGRATE_VMA_FAULT or MIGRATE_VMA_WRITE is used (or HMM_PFN_REQ_MIGRATE is combined with HMM_PFN_REQ_FAULT), can a fault on one PTE cause hmm_do_fault() to fault across the rest of the PMD range, deadlocking on already-locked folios or faulting caller-owned device-private pages back to RAM? In mm/hmm.c:hmm_vma_handle_pte(): fault: if (hmm_vma_walk->ptelocked) { lazy_mmu_mode_disable(); pte_unmap_unlock(ptep, hmm_vma_walk->ptl); hmm_vma_walk->ptelocked = false; } else { pte_unmap(ptep); } /* Fault any virtual address we were asked to fault */ return hmm_record_fault(addr, end, required_fault, walk); Here end is the end of the PMD walk range passed from hmm_vma_walk_pmd(), and hmm_do_fault() loops from addr to end calling handle_mm_fault() on every page in [addr, end): for (; addr < end; addr += PAGE_SIZE) { vm_fault_t ret; ret = handle_mm_fault(vma, addr, fault_flags, NULL); If a subsequent PTE in [addr + PAGE_SIZE, end) is a device-private entry owned by args->pgmap_owner, handle_mm_fault() will invoke do_swap_page() -> pgmap->ops->migrate_to_ram() even though hmm_vma_handle_pte() normally avoids faulting caller-owned device-private pages (and if that entry is args->fault_page, which is already locked, do_swap_page() will wait forever in softleaf_entry_wait_on_locked()). Similarly, if a subsequent PTE in [addr + PAGE_SIZE, end) already has a migration entry installed by the current thread during an earlier pass, handle_mm_fault() will call migration_entry_wait() and deadlock on the folio lock held by current. Should hmm_vma_handle_pte() pass addr + PAGE_SIZE instead of end to hmm_record_fault()? [Severity: High] Relatedly, when hmm_range_fault_locked() calls hmm_do_fault() -> handle_mm_fault() after a walk callback returns HMM_FAULT_PENDING, current still holds folio_lock on all folios collected at earlier addresses in the walk (via hmm_vma_handle_migrate_prepare() or hmm_vma_handle_migrate_prepare_pmd()). Does calling handle_mm_fault() while holding folio_lock on previously collected folios violate mm lock ordering (where mapping->invalidate_lock in filemap_fault() is acquired outside folio_lock) and risk a self-deadlock if the faulted address resolves to the same folio that was already locked at an earlier address in the range (such as a shared, KSM, or swap-cached folio mapped at two addresses)? > + > + ret = hmm_range_fault(&range); > + > + migrate_hmm_range_setup(&range); [Severity: High] By switching migrate_vma_setup() from migrate_vma_collect() to hmm_range_fault(), the walk now runs hmm_vma_walk_test() in mm/hmm.c: if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)) && vma->vm_flags & VM_READ) return 0; ... hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR); /* Skip this vma and continue processing the next vma. */ return 1; Previously, migrate_vma_collect() did not have a .test_walk callback requiring VM_READ. With this change, if a VMA containing device-private or device-coherent pages does not have VM_READ set (for example, an execute-only VMA that faults on instruction fetch, or a VMA changed to PROT_NONE via mprotect() before driver eviction or a FOLL_FORCE access), hmm_vma_walk_test() skips the VMA, migrate_hmm_range_setup() sets args->cpages = 0, and migrate_vma_setup() returns 0. In pgmap->ops->migrate_to_ram() handlers (such as nouveau_dmem_migrate_to_ram(), __drm_pagemap_migrate_to_ram(), svm_migrate_to_ram(), and dmirror_devmem_fault()), a 0 return from migrate_vma_setup() with cpages == 0 is treated as a race where another thread already migrated the page, so the handler returns 0 without resolving the device-private swap PTE and the CPU page fault retries indefinitely. Driver eviction paths also fail to migrate device pages back to RAM when VM_READ is clear. Should hmm_vma_walk_test() allow non-VM_READ VMAs when migrating (or when faulting is not requested during migration)? [Severity: Medium] Does switching migrate_vma_setup() to hmm_range_fault() also cause present pte_protnone() and pmd_protnone() pages (such as pages marked for NUMA hinting faults by automatic NUMA balancing) to be skipped when MIGRATE_VMA_FAULT is not set? In mm/hmm.c, pte_to_hmm_pfn_flags() and pmd_to_hmm_pfn_flags() return 0 (without HMM_PFN_VALID) when pte_protnone(pte) or pmd_protnone(pmd) is true: static inline unsigned long pte_to_hmm_pfn_flags(struct hmm_range *range, pte_t pte) { if (pte_none(pte) || !pte_present(pte) || pte_protnone(pte)) return 0; return pte_write(pte) ? (HMM_PFN_VALID | HMM_PFN_WRITE) : HMM_PFN_VALID; } When callers invoke migrate_vma_setup() without MIGRATE_VMA_FAULT, no fault is triggered for protnone entries, and both hmm_vma_handle_migrate_prepare() and hmm_vma_handle_migrate_prepare_pmd() bail out at: if (!(hmm_pfn[0] & HMM_PFN_VALID)) goto out; Previously, migrate_vma_collect_pmd() and migrate_vma_collect_huge_pmd() collected and migrated present protnone PTEs and THP PMDs without requiring a fault. [ ... ] > > /* > * At this point pages are locked and unmapped, and thus they have титься -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
