Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mm/hmm: broken migration entries in hmm_vma_handle_pte() and hmm_vma_walk_pmd() - [High] mm/hmm: broken state and rollback in hmm_vma_walk_pmd() on retry after -EAGAIN - [Medium] mm/hmm: hmm_vma_walk_pmd() aborts PMD walk and clobbers PFNs on split failure --
commit 25901a209351c6985ba22f8dfc40a2db03ec4bdf Author: Mika Penttilä <[email protected]> mm/hmm: do the plumbing for HMM to participate in migration This commit adds page table locking state and migration preparation plumbing to hmm_range_fault() and its pagewalk callbacks in mm/hmm.c so that PTE and PMD locks are held across entry handling and migration collection and dropped before faulting. > diff --git a/mm/hmm.c b/mm/hmm.c > index 4e9542aedc253..119e70dbca56f 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -305,14 +360,25 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, > unsigned long addr, > goto fault; > > if (softleaf_is_migration(entry)) { > - pte_unmap(ptep); > - hmm_vma_walk->last = addr; > - migration_entry_wait(walk->mm, pmdp, addr); > - return -EBUSY; > + 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; > } [Severity: High] In include/linux/hmm.h, the input request flags share the same bit positions as the output flags (HMM_PFN_REQ_FAULT == HMM_PFN_VALID, HMM_PFN_REQ_WRITE == HMM_PFN_WRITE, HMM_PFN_REQ_MIGRATE == HMM_PFN_MIGRATE), and callers using range->default_flags with range->pfn_flags_mask == 0 are not required to initialize range->hmm_pfns[] on input. In hmm_vma_handle_pte(), the !pte_present(pte) path evaluates required_fault = hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0) first: required_fault = hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0); if (!required_fault) goto out; Because of that check, if (softleaf_is_migration(entry)) is only reached when required_fault != 0 (HMM_PFN_REQ_FAULT was requested). When hmm_select_migrate(range) is non-zero, returning 0 directly here skips both migration_entry_wait() (which would require unlocking hmm_vma_walk->ptl first) and the out: label that masks *hmm_pfn to HMM_PFN_INOUT_FLAGS: out: *hmm_pfn = (*hmm_pfn & HMM_PFN_INOUT_FLAGS) | new_pfn_flags; return 0; Won't returning 0 directly on a concurrent migration entry leave stale input flags or uninitialized bits (such as HMM_PFN_REQ_FAULT / HMM_PFN_VALID and HMM_PFN_REQ_MIGRATE / HMM_PFN_MIGRATE) in *hmm_pfn, pass a migration PTE into hmm_vma_handle_migrate_prepare(), and allow hmm_range_fault() to succeed without waiting for the migration entry to resolve despite HMM_PFN_REQ_FAULT? Conversely, when HMM_PFN_REQ_FAULT is not set (such as the default in migrate_vma_setup()), if (!required_fault) goto out runs before checking softleaf_is_migration(entry). If a retry re-walks a migration PTE that was already installed by this walk, won't jumping to out: clear *hmm_pfn (and trigger rollback in the later patch "mm/hmm: implement rollback for device page migration in HMM pagewalk") while leaving the migration PTE in the page table? [ ... ] > @@ -468,46 +593,116 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp, > { > struct hmm_vma_walk *hmm_vma_walk = walk->private; > struct hmm_range *range = hmm_vma_walk->range; > - unsigned long *hmm_pfns = > - &range->hmm_pfns[(start - range->start) >> PAGE_SHIFT]; > unsigned long npages = (end - start) >> PAGE_SHIFT; > + struct mm_struct *mm = walk->vma->vm_mm; > + unsigned long *hmm_pfns, *hmm_pfns_start; > + enum migrate_vma_info minfo; > unsigned long addr = start; > + bool unmapped = false; > + unsigned long i; > pte_t *ptep; > pmd_t pmd; > + int r = 0; > > + minfo = hmm_select_migrate(range); > + hmm_pfns_start = &range->hmm_pfns[(start - range->start) >> PAGE_SHIFT]; > again: > - pmd = pmdp_get_lockless(pmdp); > - if (pmd_none(pmd)) > - return hmm_vma_walk_hole(start, end, -1, walk); > + hmm_pfns = &range->hmm_pfns[(addr - range->start) >> PAGE_SHIFT]; > + hmm_vma_walk->ptelocked = false; > + hmm_vma_walk->pmdlocked = false; > + > + if (minfo) { > + hmm_vma_walk->ptl = pmd_lock(mm, pmdp); > + hmm_vma_walk->pmdlocked = true; > + pmd = pmdp_get(pmdp); > + } else > + pmd = pmdp_get_lockless(pmdp); > + > + if (pmd_none(pmd)) { > + r = hmm_vma_walk_hole(start, end, -1, walk); > + > + if (hmm_vma_walk->pmdlocked) { > + spin_unlock(hmm_vma_walk->ptl); > + hmm_vma_walk->pmdlocked = false; > + } > + return r; > + } > > if (thp_migration_supported() && pmd_is_migration_entry(pmd)) { > - if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns, npages, 0)) { > - hmm_vma_walk->last = addr; > - pmd_migration_entry_wait(walk->mm, pmdp); > - return -EBUSY; > + if (!minfo) { > + if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns_start, > npages, 0)) { > + hmm_vma_walk->last = addr; > + pmd_migration_entry_wait(walk->mm, pmdp); > + return -EBUSY; > + } > + for (i = 0; start < end; start += PAGE_SIZE, i++) > + hmm_pfns_start[i] &= HMM_PFN_INOUT_FLAGS; > + } > + > + if (hmm_vma_walk->pmdlocked) { > + spin_unlock(hmm_vma_walk->ptl); > + hmm_vma_walk->pmdlocked = false; > } > - return hmm_pfns_fill(start, end, range, 0); > + > + return 0; > } [Severity: High] Similarly, in hmm_vma_walk_pmd(), when thp_migration_supported() && pmd_is_migration_entry(pmd) is true and minfo != 0, both the hmm_range_need_fault() check and the loop masking hmm_pfns_start[] to HMM_PFN_INOUT_FLAGS are skipped by if (!minfo). When a concurrent PMD migration entry is encountered during a migration walk, doesn't skipping that block leave hmm_pfns_start[0..npages - 1] unmasked and uninitialized, and also fail to unlock hmm_vma_walk->ptl and wait in pmd_migration_entry_wait() when a fault was requested? Neither this path nor the PTE migration entry path in hmm_vma_handle_pte() appears to be updated in the subsequent patches in the series. > - if (!pmd_present(pmd)) > - return hmm_vma_handle_absent_pmd(walk, start, end, hmm_pfns, > - pmd); > + if (pmd_trans_huge(pmd) || !pmd_present(pmd)) { > + if (!pmd_present(pmd)) { > + r = hmm_vma_handle_absent_pmd(walk, start, end, > hmm_pfns_start, > + pmd); > + // If not migrating we are done > + if (r || !minfo) { > + if (hmm_vma_walk->pmdlocked) { > + spin_unlock(hmm_vma_walk->ptl); > + hmm_vma_walk->pmdlocked = false; > + } > + return r; > + } > + } > > - if (pmd_trans_huge(pmd)) { > - /* > - * No need to take pmd_lock here, even if some other thread > - * is splitting the huge pmd we will get that event through > - * mmu_notifier callback. > - * > - * So just read pmd value and check again it's a transparent > - * huge or device mapping one and compute corresponding pfn > - * values. > - */ > - pmd = pmdp_get_lockless(pmdp); > - if (!pmd_trans_huge(pmd)) > - goto again; > + if (pmd_trans_huge(pmd)) { > + /* > + * No need to take pmd_lock here if not migrating, > + * even if some other thread is splitting the huge > + * pmd we will get that event through mmu_notifier > callback. > + * > + * So just read pmd value and check again it's a > transparent > + * huge or device mapping one and compute corresponding > pfn > + * values. > + */ > + > + if (!minfo) { > + pmd = pmdp_get_lockless(pmdp); > + if (!pmd_trans_huge(pmd)) > + goto again; > + } > > - return hmm_vma_handle_pmd(walk, addr, end, hmm_pfns, pmd); > + r = hmm_vma_handle_pmd(walk, start, end, > hmm_pfns_start, pmd); > + > + // If not migrating we are done > + if (r || !minfo) { > + if (hmm_vma_walk->pmdlocked) { > + spin_unlock(hmm_vma_walk->ptl); > + hmm_vma_walk->pmdlocked = false; > + } > + return r; > + } > + } > + > + r = hmm_vma_handle_migrate_prepare_pmd(walk, pmdp, start, end, > hmm_pfns_start); > + > + if (hmm_vma_walk->pmdlocked) { > + spin_unlock(hmm_vma_walk->ptl); > + hmm_vma_walk->pmdlocked = false; > + } > + > + return r; > + } > + > + if (hmm_vma_walk->pmdlocked) { > + spin_unlock(hmm_vma_walk->ptl); > + hmm_vma_walk->pmdlocked = false; > } > > /* > @@ -517,24 +712,64 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp, > * recover. > */ > if (pmd_bad(pmd)) { > - if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns, npages, 0)) > + if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns_start, npages, > 0)) > return -EFAULT; > - return hmm_pfns_fill(start, end, range, HMM_PFN_ERROR); > + return hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR); > } > > - ptep = pte_offset_map(pmdp, addr); > - if (!ptep) > + if (minfo) { > + ptep = pte_offset_map_lock(mm, pmdp, addr, &hmm_vma_walk->ptl); > + if (ptep) > + hmm_vma_walk->ptelocked = true; > + } else { > + ptep = pte_offset_map(pmdp, addr); > + } > + if (!ptep) { > + addr = start; > goto again; > + } > + > for (; addr < end; addr += PAGE_SIZE, ptep++, hmm_pfns++) { > - int r; > > r = hmm_vma_handle_pte(walk, addr, end, pmdp, ptep, hmm_pfns); > if (r) { > - /* hmm_vma_handle_pte() did pte_unmap() */ > + /* hmm_vma_handle_pte() did pte_unmap() / > pte_unmap_unlock */ > + if (unmapped) > + flush_tlb_range(walk->vma, start, addr); > return r; > } > + > + r = hmm_vma_handle_migrate_prepare(walk, pmdp, ptep, addr, > hmm_pfns, &unmapped); > + if (r == -EAGAIN) { > + HMM_ASSERT_UNLOCKED(hmm_vma_walk); > + if (unmapped) { > + flush_tlb_range(walk->vma, start, addr); > + unmapped = false; > + } > + goto again; > + } [Severity: High] When hmm_vma_handle_migrate_prepare() (populated in the later patch "mm/hmm: migrate collection in HMM pagewalk - pte level") encounters a PTE-mapped large folio at addr > start after order-0 pages in [start, addr) have already been collected, migrate_vma_split_folio() drops the PTE lock and returns -EAGAIN, jumping to again: with addr > start. Even with the later patch "mm/hmm: implement rollback for device page migration in HMM pagewalk", can jumping back to again: with addr > start leave stale collected state in [start, addr) or leak folio locks and references? 1. If pte_offset_map_lock() returns null (!ptep) at addr > start because the PTE table disappeared while the PTE lock was dropped, the rollback patch calls hmm_vma_handle_migrate_prepare_rollback(walk->private, addr, end, hmm_pfns) on the unwalked tail [addr, end) instead of rolling back the already-collected prefix [start, addr) at hmm_pfns_start before resetting addr = start. 2. If the PTE page table was removed or replaced while the PTE lock was dropped in migrate_vma_split_folio() (or after !ptep jumps to again:), the PMD-level paths at again: do not roll back [start, addr) when pmd_is_migration_entry(pmd) is true (which returns 0 with [start, addr) still marked HMM_PFN_VALID | HMM_PFN_MIGRATE and addr still marked HMM_PFN_VALID from hmm_vma_handle_pte()), or when hmm_vma_walk_hole(), hmm_vma_handle_absent_pmd(), hmm_vma_handle_pmd(), or pmd_bad() returns HMM_FAULT_PENDING or -EFAULT. Likewise, if the PTE table was replaced by a new PTE table while the lock was dropped, again: resumes at addr > start without rolling back or re-walking [start, addr). > + if (r) { > + /* A non -EAGAIN error here means > migrate_vma_split_folio() > + * already dropped the PTE lock and cleared ptelocked. > + */ > + HMM_ASSERT_UNLOCKED(hmm_vma_walk); > + if (unmapped) > + flush_tlb_range(walk->vma, start, addr); > + hmm_pfns_fill(addr, end, hmm_vma_walk, HMM_PFN_ERROR); > + return 0; > + } [Severity: Medium] When hmm_vma_handle_migrate_prepare() returns a non-EAGAIN error in the later patch "mm/hmm: migrate collection in HMM pagewalk - pte level" because migrate_vma_split_folio() failed to split a PTE-mapped large folio, why does hmm_vma_walk_pmd() fill [addr, end) with HMM_PFN_ERROR and return 0? First, hmm_vma_handle_pte() has already recorded the valid PFN and HMM_PFN_VALID in *hmm_pfns for addr. Overwriting addr with HMM_PFN_ERROR means callers such as nouveau_dmem_migrate_fault() (in "drm/nouveau/svm: add support for migrate on fault") that fall back to mapping present CPU pages in place when migration does not happen cannot use the PFN, unlike folio_trylock() or folio_try_share_anon_rmap_pte() failures which preserve the valid PFN without HMM_PFN_MIGRATE. Second, returning 0 immediately aborts the loop for the rest of the PMD range [addr + PAGE_SIZE, end), skipping both faulting (hmm_vma_handle_pte()) and migration collection (hmm_vma_handle_migrate_prepare()) for all remaining PTEs in that PMD and returning 0 from hmm_range_fault() with HMM_PFN_ERROR entries even when HMM_PFN_REQ_FAULT was requested. > } > - pte_unmap(ptep - 1); > + > + if (unmapped) > + flush_tlb_range(walk->vma, start, addr); > + > + if (hmm_vma_walk->ptelocked) { > + pte_unmap_unlock(ptep - 1, hmm_vma_walk->ptl); > + hmm_vma_walk->ptelocked = false; > + } else { > + pte_unmap(ptep - 1); > + } > + > return 0; > } [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
