Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] mm: memory corruption via uninitialized PFN unlock/put during rollback - [High] mm: infinite loop in hmm_range_fault_locked on failed THP split - [Medium] mm: lost per-PFN input flags on hmm rollback preventing correct retries --
commit 773397c69855b5d4adbb7a21c81d362234a475a7 Author: Mika Penttilä <[email protected]> mm/hmm: implement rollback for device page migration in HMM pagewalk During the migration pagewalk, the PTE table could be cleared and/or changed into PMD leaf or even another PTE table while dropped locks. In these cases the possibly inserted migration ptes are gone. We have to however undo the collecting done so far, so unlock the folios and drop reference taken. [ ... ] > diff --git a/include/linux/hmm.h b/include/linux/hmm.h > index 4f56f3419cb46..b08ebc1343dd9 100644 > --- a/include/linux/hmm.h > +++ b/include/linux/hmm.h > @@ -111,6 +111,28 @@ static inline unsigned int hmm_pfn_to_map_order(unsigned > long hmm_pfn) > return (hmm_pfn >> HMM_PFN_ORDER_SHIFT) & 0x1F; > } > > +/* > + * hmm_pfn_collected() - is this pfn entry prepared for migration ? > + * If collected the folio's refcount is increased and the folio > + * is locked. > + */ > +static inline bool hmm_pfn_collected(unsigned long hmm_pfn) > +{ > + return (hmm_pfn & (HMM_PFN_VALID | HMM_PFN_MIGRATE)) == > + (HMM_PFN_VALID | HMM_PFN_MIGRATE); > +} > + > +/* > + * hmm_pfn_rollback_collected() - undoes the collection of hmm_pfn > + * > + * Note for total rollback the folio's refcount has to be put > + * and folio has to be unlocked. > + */ > +static inline unsigned long hmm_pfn_rollback_collected(unsigned long hmm_pfn) > +{ > + return hmm_pfn & ~(HMM_PFN_VALID | HMM_PFN_MIGRATE | HMM_PFN_COMPOUND); > +} [Severity: Medium] Will hmm_pfn_rollback_collected() destroy the user's per-PFN request flags? When pte_offset_map_lock() fails in hmm_vma_walk_pmd(), hmm_vma_handle_migrate_prepare_rollback() is called on unprocessed entries. This clears HMM_PFN_VALID and HMM_PFN_MIGRATE here. Because input flags (HMM_PFN_REQ_FAULT and HMM_PFN_REQ_MIGRATE) share the exact same bit definitions as the output flags, this effectively destroys the user's request flags. If the outer loop retries the walk, will it see no request flags for these pages and silently skip them? > + > /* > * struct hmm_range - track invalidation lock on virtual address range > * > diff --git a/mm/hmm.c b/mm/hmm.c > index 9fdd945cc0266..daf83f8091518 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -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] Can this regression lead to memory corruption via an uninitialized PFN unlock/put? Here hmm_vma_handle_migrate_prepare_rollback() is called before the individual hmm_pfn entry is populated with new_pfn_flags, meaning it reads and acts on the user's input flags. If a user calls hmm_range_fault() to request migration using per-PFN flags (setting HMM_PFN_REQ_MIGRATE | HMM_PFN_REQ_FAULT, which map directly to HMM_PFN_MIGRATE | HMM_PFN_VALID), hmm_pfn_collected() will incorrectly evaluate to true. This causes hmm_vma_handle_migrate_prepare_rollback() to extract the PFN part (typically 0), cast it to a folio, and attempt to unlock/put it: hmm_vma_handle_migrate_prepare_rollback() { ... folio = page_folio(hmm_pfn_to_page(hmm_pfn[i])); if (folio != fault_folio) folio_unlock(folio); folio_put(folio); ... } [ ... ] > @@ -1117,6 +1190,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] Is there an infinite loop in hmm_range_fault_locked() when a THP split fails? When hmm_vma_handle_migrate_prepare_pmd() attempts to split a PMD-mapped THP via __migrate_vma_split_folio(), the split can fail and return -EBUSY. It propagates this -EBUSY directly to hmm_vma_walk_pmd() without updating hmm_vma_walk->last: hmm_vma_handle_migrate_prepare_pmd() { ... r = __migrate_vma_split_folio(folio, migrate->fault_page); if (r) return r; hmm_vma_walk->last = start; return -EBUSY; } However, hmm_vma_walk_pmd() handles -EBUSY as a success sentinel meaning "split succeeded, please re-walk": /* * 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; This propagates the -EBUSY to walk_page_range(), and the outer loop in hmm_range_fault_locked() restarts the walk from hmm_vma_walk.last (which was not advanced). It encounters the exact same THP, fails to split it again, and loops infinitely. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
