Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] mm/hmm: stale hmm_pfns left across PMD split re-walk in hmm_vma_walk_pmd() - [Medium] mm/hmm: hmm_vma_handle_migrate_prepare_pmd() clobbers valid PFNs on skip --
commit 19d71de5ba9a8df49f896d9047f6e5016503f7dc Author: Mika Penttilä <[email protected]> mm/hmm: migrate collection in HMM pagewalk - pmd level This commit implements hmm_vma_handle_migrate_prepare_pmd() and integrates it into hmm_vma_walk_pmd() to prepare PMD-level THP and device-private entries for migration or split them for PTE re-walking. > diff --git a/mm/hmm.c b/mm/hmm.c > index 0ce947d0e5727..5891efee3e182 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -548,14 +536,145 @@ static int migrate_vma_split_folio(struct folio *folio, > return 0; > } > > +/** > + * migrate_vma_split_folio() - drop the pte lock and split a THP folio > + * @folio: the folio to split > + * @fault_page: struct page associated with the fault if any > + * @hmm_vma_walk: walk in progress > + * @ptep: pte_t * for unmap and unlock ptl > + * > + * Returns 0 on success > + */ > +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); > + > + 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); > +} > + > static int hmm_vma_handle_migrate_prepare_pmd(const struct mm_walk *walk, > pmd_t *pmdp, > unsigned long start, > unsigned long end, > unsigned long *hmm_pfn) > { [ ... ] > + folio_get(folio); > + > + if (folio != fault_folio && unlikely(!folio_trylock(folio))) { > + folio_put(folio); > + hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR); > + return 0; > + } > + > + if (thp_migration_supported() && > + (migrate->flags & MIGRATE_VMA_SELECT_COMPOUND) && > + (IS_ALIGNED(start, HPAGE_PMD_SIZE) && > + IS_ALIGNED(end, HPAGE_PMD_SIZE))) { > + struct page_vma_mapped_walk pvmw = { > + .ptl = hmm_vma_walk->ptl, > + .address = start, > + .pmd = pmdp, > + .vma = walk->vma, > + }; > + > + hmm_pfn[0] |= HMM_PFN_MIGRATE | HMM_PFN_COMPOUND; > + > + r = set_pmd_migration_entry(&pvmw, folio_page(folio, 0)); > + if (r) { > + hmm_pfn[0] &= ~(HMM_PFN_MIGRATE | HMM_PFN_COMPOUND); > + goto split; /* fall back to splitting the pmd */ > + } > + for (i = 1, start += PAGE_SIZE; start < end; start += > PAGE_SIZE, i++) > + hmm_pfn[i] &= HMM_PFN_INOUT_FLAGS; > + > + } else { > + goto split; /* fall back to splitting the pmd */ > + } > + > +out: > + return r; > + > +split: > + spin_unlock(hmm_vma_walk->ptl); > + hmm_vma_walk->pmdlocked = false; > + > + /* > + * folio_get() above took an extra reference. For the fault folio the > + * caller still holds a reference and the lock, which is the > + * precondition of __migrate_vma_split_folio(), so drop the extra one. > + */ > + if (folio == fault_folio) > + folio_put(folio); > + > + r = __migrate_vma_split_folio(folio, migrate->fault_page); > + if (r) > + return -EFAULT; > + > + hmm_vma_walk->last = start; > + return -EBUSY; > } [Severity: High] Can returning -EBUSY from the split path in hmm_vma_handle_migrate_prepare_pmd() leave stale pre-split PMD entries in range->hmm_pfns[] across the re-walk? When migration is enabled (in the later commit "mm: enable device page migration from HMM pagewalk"), hmm_vma_walk_pmd() first calls hmm_vma_handle_pmd() or hmm_vma_handle_absent_pmd() before calling hmm_vma_handle_migrate_prepare_pmd(): mm/hmm.c:hmm_vma_walk_pmd() hmm_vma_handle_pmd() / hmm_vma_handle_absent_pmd() for (i = 0; addr < end; addr += PAGE_SIZE, i++, pfn++) { hmm_pfns[i] &= HMM_PFN_INOUT_FLAGS; hmm_pfns[i] |= pfn | cpu_flags; } hmm_vma_handle_migrate_prepare_pmd() This populates hmm_pfns_start[0..npages-1] with the pre-split PMD PFNs, HMM_PFN_VALID, HMM_PFN_WRITE, and hmm_pfn_flags_order(PMD_SHIFT - PAGE_SHIFT). When hmm_vma_handle_migrate_prepare_pmd() then takes the split path (for example, when MIGRATE_VMA_SELECT_COMPOUND is not set, [start, end) is smaller than HPAGE_PMD_SIZE, or set_pmd_migration_entry() fails), it drops hmm_vma_walk->ptl, splits the folio via __migrate_vma_split_folio() (which also unlocks the folio when folio != fault_folio), sets hmm_vma_walk->last = start, and returns -EBUSY without clearing hmm_pfns_start[0..npages-1]. This breaks the hmm_range_fault_locked() expectation that range->hmm_pfns[] entries at or after hmm_vma_walk.last have not been overwritten with output flags when -EBUSY is returned. On the subsequent PTE re-walk of [start, end), if any split page is concurrently turned into a migration entry while the page table and folio locks were dropped (with HMM_PFN_REQ_FAULT set), hmm_vma_handle_pte() returns 0 without updating *hmm_pfn: mm/hmm.c:hmm_vma_handle_pte() if (softleaf_is_migration(entry)) { if (!hmm_select_migrate(range)) { ... return -EBUSY; } return 0; } and hmm_vma_handle_migrate_prepare() sees !pte_present(pte) and !softleaf_is_device_private(entry) and also returns 0 without updating *hmm_pfn (similarly, if the PMD became a PMD migration entry, hmm_vma_walk_pmd() returns 0 without clearing hmm_pfns_start). The later commit "mm/hmm: implement rollback for device page migration in HMM pagewalk" only rolls back entries that have both HMM_PFN_VALID and HMM_PFN_MIGRATE set, so these pre-split entries (which lack HMM_PFN_MIGRATE) remain in range->hmm_pfns[], allowing callers that map non-migrated HMM PFNs in place (such as nouveau_dmem_migrate_fault()) to map a stale 2MB host range into the device MMU. Furthermore, if a caller sets range->pfn_flags_mask, overwriting hmm_pfns_start[0..npages-1] with HMM_PFN_VALID (which shares its bit position with HMM_PFN_REQ_FAULT) and HMM_PFN_WRITE (which shares its bit position with HMM_PFN_REQ_WRITE) before the split clobbers the per-page request flags for the PTE re-walk. > @@ -958,6 +1077,17 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp, > hmm_vma_walk->pmdlocked = false; > } > > + /* > + * hmm_vma_handle_migrate_prepare_pmd() splits the huge pmd in > + * place when needed and returns -EBUSY to re-walk the range as > + * PTEs; any other error means the split failed. > + */ > + if (r == -EBUSY) > + return -EBUSY; > + if (r) { > + /* Split not successful, skip */ > + return hmm_pfns_fill(start, end, hmm_vma_walk, > HMM_PFN_ERROR); > + } > return r; > } [Severity: Medium] Should the folio_trylock() failure path in hmm_vma_handle_migrate_prepare_pmd() and the split failure path here in hmm_vma_walk_pmd() preserve the valid PFNs in range->hmm_pfns[] instead of overwriting them with HMM_PFN_ERROR? Before hmm_vma_handle_migrate_prepare_pmd() is called, hmm_vma_handle_pmd() (or hmm_vma_handle_absent_pmd()) has already populated hmm_pfns_start[0..npages-1] with the valid PFNs (HMM_PFN_VALID without HMM_PFN_MIGRATE). However, in two best-effort migration skip cases, those valid PFNs are clobbered with HMM_PFN_ERROR: 1. In hmm_vma_handle_migrate_prepare_pmd(), when folio_trylock(folio) fails while holding hmm_vma_walk->ptl, it calls hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR) even though the PMD lock was never dropped and the mapping is still valid. 2. When compound migration is not selected or [start, end) is smaller than HPAGE_PMD_SIZE (such as single-page faults in nouveau_dmem_migrate_fault() in the later commit "drm/nouveau/svm: add support for migrate on fault"), hmm_vma_handle_migrate_prepare_pmd() jumps to split and calls __migrate_vma_split_folio(). If split_folio() fails (for example, when the THP is pinned or has extra references), it returns -EFAULT and hmm_vma_walk_pmd() overwrites the range with HMM_PFN_ERROR. By contrast, the other non-migrating exits in hmm_vma_handle_migrate_prepare_pmd() (goto out) and folio_trylock() failure at the PTE level in hmm_vma_handle_migrate_prepare(): mm/hmm.c:hmm_vma_handle_migrate_prepare() if (fault_folio == folio || folio_trylock(folio)) { ... } else { folio_put(folio); } out: return 0; leave range->hmm_pfns[] with the valid PFNs and without HMM_PFN_MIGRATE. Because migrate_hmm_range_setup() only collects entries with HMM_PFN_MIGRATE set, leaving the valid PFNs intact still skips migration while allowing callers such as nouveau_dmem_migrate_fault() to map the present THP in place instead of treating it as an error and falling back to a redundant page table walk and GPU PFNMAP ioctl. Could both skip paths return 0 without calling hmm_pfns_fill(..., HMM_PFN_ERROR)? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
