Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] mm/hmm: premature rollback of migration PTEs in hmm_vma_handle_pte() - [High] mm/hmm: incomplete rollback on mid-PMD lock drop in hmm_vma_walk_pmd() - [High] mm/hmm: missing rollback on fault and PMD migration in hmm_vma_walk_pmd() --
commit 5c75620371e34a43110051163d33f29ab4f700b4 Author: Mika Penttilä <[email protected]> mm/hmm: implement rollback for device page migration in HMM pagewalk This commit adds rollback handling during HMM migration page walks to unlock and drop references on previously collected folios if page tables change while locks are dropped. > diff --git a/mm/hmm.c b/mm/hmm.c > index 36a4edde9823c..5a1471fd5d288 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -282,6 +289,8 @@ static int hmm_vma_handle_pmd(struct mm_walk *walk, > unsigned long addr, > return hmm_record_fault(addr, end, required_fault, walk); > } > > + hmm_vma_handle_migrate_prepare_rollback(hmm_vma_walk, addr, > + end, hmm_pfns); > pfn = pmd_pfn(pmd) + ((addr & ~PMD_MASK) >> PAGE_SHIFT); > for (i = 0; addr < end; addr += PAGE_SIZE, i++, pfn++) { > hmm_pfns[i] &= HMM_PFN_INOUT_FLAGS; [Severity: High] What happens if entries in hmm_pfns were previously collected, locks were dropped, and on re-walk the page table entry now requires a fault, fails with -EFAULT, or is a PMD migration entry? In hmm_vma_handle_pmd(), if required_fault is non-zero, it returns hmm_record_fault() before calling hmm_vma_handle_migrate_prepare_rollback(). Several other paths similarly return before rolling back previously collected entries in range->hmm_pfns: - hmm_vma_walk_hole() returns -EFAULT (when !walk->vma) or hmm_record_fault() when required_fault is non-zero before reaching hmm_pfns_fill(). - hmm_vma_handle_absent_pmd() returns hmm_record_fault() or -EFAULT when required_fault is non-zero without calling hmm_vma_handle_migrate_prepare_rollback(). - hmm_vma_handle_pte() jumps to the fault: label or returns -EFAULT without rolling back *hmm_pfn. - hmm_vma_walk_pmd() returns -EFAULT on pmd_bad(pmd) when hmm_range_need_fault() is non-zero, and returns 0 when thp_migration_supported() && pmd_is_migration_entry(pmd) is true with minfo non-zero, without rolling back collected PTE entries in hmm_pfns_start. If hmm_record_fault() returns HMM_FAULT_PENDING while the current task still holds folio_lock() on previously collected folios in that range, can hmm_do_fault() -> handle_mm_fault() deadlock trying to lock the same folio, or can the early returns leave stale collected folios locked and referenced in range->hmm_pfns? > @@ -412,6 +421,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; > [Severity: Critical] Could reaching the out: label in hmm_vma_handle_pte() prematurely roll back a migration PTE that this walk already installed? In hmm_vma_handle_pte(), when !pte_present(pte) is true, the required_fault check runs before softleaf_is_migration(entry): if (!pte_present(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)) { ... return -EBUSY; } return 0; } During a migration walk without HMM_PFN_REQ_FAULT (such as migrate_vma_setup() where range.default_flags is HMM_PFN_REQ_MIGRATE), hmm_vma_handle_migrate_prepare() locks the folio, takes a reference, installs a migration PTE, and marks *hmm_pfn with HMM_PFN_VALID | HMM_PFN_MIGRATE. If the walk restarts over that range (for example, after dropping locks in migrate_vma_split_folio() or on an -EBUSY retry), hmm_vma_handle_pte() sees !pte_present(pte) for its own migration PTE. Because HMM_PFN_REQ_FAULT is not set, hmm_pte_need_fault() returns 0 and jumps to out: with new_pfn_flags = 0. At out:, hmm_vma_handle_migrate_prepare_rollback() unlocks the folio, calls folio_put(), and clears HMM_PFN_VALID | HMM_PFN_MIGRATE from *hmm_pfn while the migration PTE remains installed in the page table, so remove_migration_ptes() will never be called for it. Conversely, when HMM_PFN_REQ_FAULT is set (required_fault != 0), the softleaf_is_migration(entry) branch returns 0 whenever hmm_select_migrate(range) is non-zero without checking that hmm_pfn_collected(*hmm_pfn) is true and that softleaf_to_pfn(entry) matches the PFN in *hmm_pfn. If the migration PTE belongs to a concurrent migration (at an uncollected slot or in a replacement PTE table), shouldn't hmm_vma_handle_pte() roll back any mismatched *hmm_pfn and wait via migration_entry_wait()? [ ... ] > @@ -1117,6 +1193,9 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp, > 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); [Severity: High] Does this rollback miss entries collected in [start, addr) when the PTE lock is dropped partway through a PMD? In hmm_vma_walk_pmd(), the PTE loop advances addr and hmm_pfns from start toward end while collecting folios into hmm_pfns_start. If hmm_vma_handle_migrate_prepare() hits a large folio at addr > start, migrate_vma_split_folio() drops the PTE lock and returns -EAGAIN, and hmm_vma_walk_pmd() jumps to again: without resetting addr = start: again: hmm_pfns = &range->hmm_pfns[(addr - range->start) >> PAGE_SHIFT]; ... 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); } } if (!ptep) { addr = start; goto again; } When ptep is NULL at addr > start, hmm_vma_handle_migrate_prepare_rollback() is called with addr and hmm_pfns before addr is reset to start, so it only scans the uncollected tail [addr, end) and leaves folios collected in [start, addr) (at hmm_pfns_start) locked and referenced. In addition, if the PTE table was cleared and replaced by another PTE table while the lock was dropped in migrate_vma_split_folio() (or during a mid-PMD fault where hmm_record_fault() sets hmm_vma_walk->last = addr), pte_offset_map_lock() succeeds at addr > start on the new PTE table and resumes from addr without re-walking or rolling back [start, addr). Should mid-PMD lock drops restart from start (with hmm_pfns_start) so that [start, addr) is re-validated? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
