Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mm/hmm: missing rollback of prior entries when restarting after fault - [High] mm/hmm: missing rollback and incomplete walk on folio split failure - [High] mm/hmm: self-deadlock on -EBUSY retry due to memset() of hmm_pfns --
commit 57b87c4290e06aea8db775c69fa86c746acfe43d Author: Mika Penttilä <[email protected]> mm/hmm: implement rollback for device page migration in HMM pagewalk This commit adds rollback handling to unlock and unreference already collected folios if page tables vanish or change while page table locks are dropped during a migration pagewalk. It also skips migration PTEs installed earlier in the same walk when restarting a PMD walk. > diff --git a/mm/hmm.c b/mm/hmm.c > index 690b73908f34e..1c8a3fa29c4e6 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -331,6 +344,15 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, > unsigned long addr, > if (!pte_present(pte)) { > const softleaf_t entry = softleaf_from_pte(pte); > > + /* > + * Our own migration entry, collected earlier in this walk: > + * keep the collected pfn, don't fault or roll it back. > + */ > + 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; [Severity: High] When hmm_range_fault_locked() aborts with -EBUSY (or a caller retries hmm_range_fault() after mmu_interval_read_retry() trips) after collecting earlier pages in the range, those folios remain locked, referenced, and mapped as migration PTEs. When the caller retries hmm_range_fault() with the same range and hmm_pfns array, 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)); } ... } On the retry walk, hmm_pfn_collected(*hmm_pfn) evaluates to false for the migration PTEs installed by the current thread on the previous attempt. Does this cause hmm_vma_handle_pte() and hmm_vma_walk_pmd() to treat our own migration entries as belonging to another thread and call hmm_do_fault()->handle_mm_fault()->migration_entry_wait() on folios already locked by current, resulting in a self-deadlock? > + > /* > * Don't fault in device private pages owned by the caller, > * just report the PFN. > @@ -416,6 +438,9 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, > unsigned long addr, > > new_pfn_flags = pte_pfn(pte) | cpu_flags; > 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; > > @@ -427,7 +452,19 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, > unsigned long addr, > } else { > pte_unmap(ptep); > } > - /* Fault any virtual address we were asked to fault */ > + /* > + * This pte needs a fault, so it is not our migration entry: a pfn > + * collected here earlier is gone, roll it back before faulting. > + */ > + hmm_vma_handle_migrate_prepare_rollback(hmm_vma_walk, addr, > + addr + PAGE_SIZE, hmm_pfn); > + /* > + * Fault any virtual address we were asked to fault. When migrating, > + * only this page: later ptes may be our own migration entries or > + * caller owned device private pages, which must not be faulted. > + */ > + if (hmm_select_migrate(range)) > + end = addr + PAGE_SIZE; > return hmm_record_fault(addr, end, required_fault, walk); > } [Severity: High] As reported by Sashiko on v16 [1], when a PTE fault occurs at addr after earlier PTEs at addresses below addr have already been collected by hmm_vma_handle_migrate_prepare(), the fault path drops hmm_vma_walk->ptl, rolls back only [addr, addr + PAGE_SIZE), and calls hmm_record_fault(), which sets hmm_vma_walk->last = addr and returns HMM_FAULT_PENDING. In hmm_range_fault_locked(), HMM_FAULT_PENDING disarms the migration invalidation range via hmm_vma_migrate_disarm() and runs hmm_do_fault() without holding any page table spinlock, then resumes walk_page_range() at hmm_vma_walk.last: 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); ... } Because walk_page_range() restarts at addr rather than range->start, hmm_vma_walk_pmd() is invoked with start = addr (and any earlier PMDs in [range->start, addr) are skipped completely). If the migration PTEs or PTE page table covering [range->start, addr) were zapped or replaced while locks were dropped during hmm_do_fault(), can those earlier collected entries in range->hmm_pfns ever be revisited or rolled back, or will they leak folio locks and references? [ ... ] > @@ -1192,6 +1311,12 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp, > flush_tlb_range(walk->vma, start, addr); > unmapped = false; > } > + /* > + * The pte lock was dropped and the pte table may have > + * changed: re-walk from start. Our own migration > entries > + * are kept, anything else collected is rolled back. > + */ > + addr = start; > goto again; > } > if (r) { [Severity: High] When hmm_vma_handle_migrate_prepare() attempts to split a large folio via migrate_vma_split_folio(), it unconditionally drops hmm_vma_walk->ptl and sleeps on folio_lock() before calling __migrate_vma_split_folio(): mm/hmm.c:migrate_vma_split_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 __migrate_vma_split_folio() fails, hmm_vma_handle_migrate_prepare() returns -EFAULT, and the if (r) branch in hmm_vma_walk_pmd() flushes the TLB and returns 0 immediately: mm/hmm.c:hmm_vma_walk_pmd() { ... if (r) { /* * Splitting the folio failed; migrate_vma_split_folio() * already dropped the PTE lock. Don't migrate the rest * of the pmd range; addr keeps its valid pfn. */ HMM_ASSERT_UNLOCKED(hmm_vma_walk); if (unmapped) flush_tlb_range(walk->vma, start, addr); return 0; } ... } Since the PTE lock was dropped while waiting on folio_lock(), if migration PTEs or the PTE table in [start, end) vanished during that window, won't returning 0 without re-walking [start, end) skip rolling back those collected folios, and also leave range->hmm_pfns unpopulated for the remaining addresses in [addr + PAGE_SIZE, end)? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
