Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mm/hmm: split of non-anonymous large folios in hmm_vma_handle_migrate_prepare() - [High] mm/hmm: early PMD walk termination on folio split failure in hmm_vma_walk_pmd() - [High] mm/hmm: reject KSM folios in hmm_vma_handle_migrate_prepare() --
commit 9b85d918ada9ba7990835bcd807b9973c5976885 Author: Mika Penttilä <[email protected]> mm/hmm: migrate collection in HMM pagewalk - pte level This commit implements PTE-level migration preparation and collection in hmm_vma_handle_migrate_prepare(), along with migrate_vma_split_folio() to split PTE-mapped large folios during an HMM page table walk. > diff --git a/mm/hmm.c b/mm/hmm.c > index 854d2c090b20c..ec9280ef26822 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -495,6 +495,71 @@ static int hmm_vma_handle_absent_pmd(struct mm_walk > *walk, unsigned long start, > #endif /* CONFIG_ARCH_HAS_PMD_SOFTLEAVES */ > > #ifdef CONFIG_DEVICE_MIGRATION [ ... ] > @@ -512,8 +582,230 @@ static int hmm_vma_handle_migrate_prepare(const struct > mm_walk *walk, > unsigned long *hmm_pfn, > bool *unmapped) > { > - // TODO: implement migration entry insertion > + struct hmm_vma_walk *hmm_vma_walk = walk->private; > + struct hmm_range *range = hmm_vma_walk->range; > + struct migrate_vma *migrate = range->migrate; [ ... ] > + if (folio_test_large(folio)) { > + int ret; > + > + ret = migrate_vma_split_folio(folio, > + migrate->fault_page, > + hmm_vma_walk, > + ptep); > + if (ret) > + goto out_error; > + return -EAGAIN; > + } > + > + pfn = page_to_pfn(page); > + if (softleaf_is_device_private_write(entry)) > + writable = true; > + } else { > + pfn = pte_pfn(pte); > + if (is_zero_pfn(pfn) && > + (minfo & MIGRATE_VMA_SELECT_SYSTEM)) { > + /* Like pte_none(): only anonymous memory can be > populated */ > + if (!vma_is_anonymous(walk->vma)) > + goto out; > + *hmm_pfn &= HMM_PFN_INOUT_FLAGS; > + *hmm_pfn |= HMM_PFN_MIGRATE; > + goto out; > + } > + page = vm_normal_page(walk->vma, addr, pte); > + if (page && !is_zone_device_page(page) && > + !(minfo & MIGRATE_VMA_SELECT_SYSTEM)) { > + goto out; > + } else if (page && is_device_coherent_page(page)) { > + pgmap = page_pgmap(page); > + > + if (!(minfo & > + MIGRATE_VMA_SELECT_DEVICE_COHERENT) || > + pgmap->owner != migrate->pgmap_owner) > + goto out; > + } > + > + folio = page ? page_folio(page) : NULL; > + if (folio && folio_test_large(folio)) { > + int ret; > + > + ret = migrate_vma_split_folio(folio, > + migrate->fault_page, > + hmm_vma_walk, > + ptep); > + if (ret) > + goto out_error; > + return -EAGAIN; > + } > + > + writable = pte_write(pte); > + } [Severity: High] When walking a private file-backed VMA (which can contain a mix of copy-on-write anonymous pages and read-only pagecache pages), will this attempt to split non-anonymous large pagecache folios before checking folio_test_anon()? In the pte_present(pte) branch of hmm_vma_handle_migrate_prepare(), migrate_vma_split_folio() is called on any large folio before verifying that the folio is anonymous. If a PTE maps a read-only large file-backed folio in the page cache, migrate_vma_split_folio() drops the PTE lock, takes the sleeping folio_lock(folio) on the pagecache folio while already holding locks on previously collected anonymous folios, and calls split_folio(). For file-backed folios, split_folio() -> __folio_freeze_split_file() takes i_mmap_lock_read() and unmaps the folio without remapping it, leaving the PTEs as pte_none() on the -EAGAIN retry, only for the resulting order-0 pagecache folio to be skipped by the !folio_test_anon() check. Furthermore, if the filesystem sets mapping_min_folio_order() > 0, split_folio() fails with -EINVAL (triggering VM_WARN_ONCE() in __folio_split()) and jumps to out_error. Should the !page || !folio_test_anon(folio) check happen before attempting to split the folio in the present PTE path? > + > + /* Only anonymous memory can be migrated to device memory */ > + if (!page || !folio_test_anon(page_folio(page))) > + goto out; [Severity: High] Can KSM folios pass this check and be collected for device migration? In include/linux/page-flags.h, FOLIO_MAPPING_KSM is defined as: FOLIO_MAPPING_ANON | FOLIO_MAPPING_ANON_KSM Because FOLIO_MAPPING_ANON is set on KSM folios, folio_test_anon() returns true for them. When hmm_range_fault() runs without HMM_PFN_REQ_WRITE (for example during migrate_vma_setup() or a read fault with migration), hmm_vma_handle_pte() does not break CoW on a read-only KSM PTE, and hmm_vma_handle_migrate_prepare() locks and collects the KSM folio. If the same KSM folio is mapped a second time later in the walked range, hmm_vma_handle_pte() marks the second entry HMM_PFN_VALID before folio_trylock() fails in hmm_vma_handle_migrate_prepare(), and try_to_migrate() later unmaps that second mapping via rmap_walk_ksm(), leaving a stale PFN in range->hmm_pfns. More importantly, migrating a KSM folio to MEMORY_DEVICE_PRIVATE memory updates stable_node->kpfn via folio_migrate_ksm() to a non-CPU-accessible ZONE_DEVICE PFN and installs device-private swap entries into other processes sharing the KSM page, causing ksmd to fault when it later accesses the page in memcmp_pages(). Should hmm_vma_handle_migrate_prepare() also reject folio_test_ksm(folio)? [ ... ] > + set_pte_at(mm, addr, ptep, swp_pte); > + folio_remove_rmap_pte(folio, page, walk->vma); > + folio_put(folio); > + *hmm_pfn |= HMM_PFN_VALID | HMM_PFN_MIGRATE; > + if (pte_present(pte)) > + *unmapped = true; > + } else { > + folio_put(folio); > + } > +out: > return 0; > +out_error: > + return -EFAULT; > } [Severity: High] As reported by Sashiko on v16 [1], when migrate_vma_split_folio() fails in hmm_vma_handle_migrate_prepare() and jumps to out_error (-EFAULT), does the caller in hmm_vma_walk_pmd() abort the PMD walk too early and leave the remaining pages in the PMD unprocessed? In hmm_vma_walk_pmd(), the PTE loop handles non-zero returns from hmm_vma_handle_migrate_prepare() by immediately returning 0: for (; addr < end; addr += PAGE_SIZE, ptep++, hmm_pfns++) { r = hmm_vma_handle_pte(walk, addr, end, pmdp, ptep, hmm_pfns); ... r = hmm_vma_handle_migrate_prepare(walk, pmdp, ptep, addr, hmm_pfns, &unmapped); if (r == -EAGAIN) { ... goto again; } if (r) { HMM_ASSERT_UNLOCKED(hmm_vma_walk); if (unmapped) flush_tlb_range(walk->vma, start, addr); return 0; } } Because hmm_vma_handle_pte() is called inside the same per-page loop, returning 0 from hmm_vma_walk_pmd() when a split fails at addr skips both hmm_vma_handle_pte() and hmm_vma_handle_migrate_prepare() for all remaining addresses in [addr + PAGE_SIZE, end) of the PMD. As a result, hmm_range_fault() can return 0 (success) while range->hmm_pfns[] entries for [addr + PAGE_SIZE, end) remain zeroed without HMM_PFN_VALID, and pages in that tail of the PMD are neither faulted in (when HMM_PFN_REQ_FAULT is requested) nor migrated. [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
