Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mm/hmm: dead range check in hmm_vma_capture_migrate_range() allows partial VMA - [High] mm/hmm: mmu_notifier_invalidate_range_start() held across hmm_do_fault() - [Medium] mm/hmm: hmm_vma_walk_test() captures VMA for migration before checking vm_flags --
commit ca535286660d47fc1830230d8f80c9d3e5f37db0 Author: Mika Penttilä <[email protected]> mm/hmm: preparations for HMM to participate in migration This commit prepares hmm_range_fault() to support page migration by saving the resolved VMA and range during the page walk, rejecting lock-drop mode when migrating, and invoking MMU notifier invalidations around the walk. > diff --git a/mm/hmm.c b/mm/hmm.c > index 2f1e98c6b6440..4e9542aedc253 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -400,6 +409,58 @@ static int hmm_vma_handle_absent_pmd(struct mm_walk > *walk, unsigned long start, > } > #endif /* CONFIG_ARCH_HAS_PMD_SOFTLEAVES */ > > +static int hmm_vma_capture_migrate_range(unsigned long start, > + unsigned long end, > + struct mm_walk *walk) > +{ > + struct hmm_vma_walk *hmm_vma_walk = walk->private; > + struct hmm_range *range = hmm_vma_walk->range; > + > + if (!hmm_select_migrate(range)) > + return 0; > + > + if (hmm_vma_walk->vma && (hmm_vma_walk->vma != walk->vma)) > + return -ERANGE; > + > + hmm_vma_walk->vma = walk->vma; > + > + if (end - start > range->end - range->start) > + return -ERANGE; [Severity: High] Can this condition in mm/hmm.c:hmm_vma_capture_migrate_range() ever be true? In hmm_range_fault_locked() -> walk_page_range() -> walk_page_range_mm_unsafe() -> walk_page_test() -> hmm_vma_walk_test(), the walker clamps the VMA subrange to the requested walk bounds: start >= hmm_vma_walk.last >= range->start end = min(range->end, vma->vm_end) <= range->end Because end - start is always less than or equal to range->end - range->start, this check never triggers. If [range->start, range->end) starts with an unmapped hole (without HMM_PFN_REQ_FAULT set on the hole entries), walk_page_range_mm_unsafe() calls hmm_vma_walk_hole() for [range->start, vma->vm_start) and then invokes hmm_vma_walk_test() with start == vma->vm_start > range->start. hmm_vma_capture_migrate_range() accepts this partial VMA and records hmm_vma_walk->start = vma->vm_start, which hmm_vma_post_range_fault() writes to range->migrate->start via hmm_fill_migrate_vma(). However, the page walk callbacks index range->hmm_pfns[] relative to range->start: &range->hmm_pfns[(start - range->start) >> PAGE_SHIFT] When migrate_hmm_range_setup() later reads range->hmm_pfns[0] for migrate->start, the PFN array is misaligned with the virtual address range. Additionally, if the entire [range->start, range->end) falls in an unmapped hole without fault requests, hmm_vma_walk_test() is never called and range->migrate->vma remains uninitialized. Should this check instead verify that the single VMA covers the entire requested range (for example, vma->vm_start > range->start || vma->vm_end < range->end), and should hmm_vma_walk_hole() reject holes when hmm_select_migrate(range) is enabled? > + > + if (!hmm_vma_walk->mmu_armed) { > + hmm_vma_walk->start = start; > + hmm_vma_walk->end = end; > + mmu_notifier_range_init_owner(&hmm_vma_walk->mmu_range, > MMU_NOTIFY_MIGRATE, 0, > + walk->vma->vm_mm, start, end, > + range->dev_private_owner); > + mmu_notifier_invalidate_range_start(&hmm_vma_walk->mmu_range); > + hmm_vma_walk->mmu_armed = true; > + } > + > + return 0; > +} > + > +static void hmm_vma_post_range_fault(struct hmm_vma_walk *hmm_vma_walk) > +{ > + > + struct hmm_range *range = hmm_vma_walk->range; > + > + if (hmm_select_migrate(range) && > + hmm_vma_walk->mmu_armed) { > + /* > + * The migrate_vma path has the following initialized, > + * so take care of fault path below. > + */ > + if (range->notifier) { > + hmm_fill_migrate_vma(range, > + hmm_vma_walk->vma, > + hmm_vma_walk->start, > + hmm_vma_walk->end); > + } > + mmu_notifier_invalidate_range_end(&hmm_vma_walk->mmu_range); > + } > +} > + > static int hmm_vma_walk_pmd(pmd_t *pmdp, > unsigned long start, > unsigned long end, > @@ -594,6 +655,11 @@ static int hmm_vma_walk_test(unsigned long start, > unsigned long end, > struct hmm_vma_walk *hmm_vma_walk = walk->private; > struct hmm_range *range = hmm_vma_walk->range; > struct vm_area_struct *vma = walk->vma; > + int r; > + > + r = hmm_vma_capture_migrate_range(start, end, walk); > + if (r) > + return r; > > if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)) && > vma->vm_flags & VM_READ) [Severity: Medium] In mm/hmm.c:hmm_vma_walk_test(), is it intentional that hmm_vma_capture_migrate_range() runs before validating vma->vm_flags? When walk->vma has VM_IO or VM_PFNMAP set, or lacks VM_READ, calling hmm_vma_capture_migrate_range() first records hmm_vma_walk->vma = walk->vma and invokes mmu_notifier_invalidate_range_start() on the unsupported VMA. If hmm_range_need_fault() then returns 0, hmm_vma_walk_test() fills range->hmm_pfns with HMM_PFN_ERROR and returns 1 (skipping the VMA), after which hmm_vma_post_range_fault() populates range->migrate->vma with the unsupported VMA. Also, in the fault migration path where range->notifier is set and migrate_vma_setup() is not used, nothing in hmm_vma_walk_test() or hmm_vma_capture_migrate_range() rejects is_vm_hugetlb_page(vma), (vma->vm_flags & VM_SPECIAL), or vma_is_dax(vma), which migrate_vma_setup() rejects with -EINVAL. Should VMA eligibility for both HMM and migration be verified before capturing the VMA and arming the MMU notifier? [ ... ] > @@ -694,19 +760,31 @@ static int hmm_range_fault_locked(struct hmm_range > *range, bool *locked) > { > struct hmm_vma_walk hmm_vma_walk = { > .range = range, > - .locked = locked, > + .mmlocked = locked, > .last = range->start, > }; > - struct mm_struct *mm = range->notifier->mm; > + /* > + * Could be serving a device fault or come from migrate > + * entry point. For the former we have not resolved the vma > + * yet, and the latter we don't have a notifier (but have a vma). > + * > + */ > + struct mm_struct *mm = hmm_range_fault_mm(range); > int ret; > > + /* Migration is incompatible with mmap lock drop */ > + if (locked && hmm_select_migrate(range)) > + return -EINVAL; > + > mmap_assert_locked(mm); > > do { > /* If range is no longer valid force retry. */ > - if (mmu_interval_check_retry(range->notifier, > - range->notifier_seq)) > - return -EBUSY; > + if (range->notifier && mmu_interval_check_retry(range->notifier, > + > range->notifier_seq)) { > + ret = -EBUSY; > + break; > + } > ret = walk_page_range(mm, hmm_vma_walk.last, range->end, > &hmm_walk_ops, &hmm_vma_walk); > /* > @@ -732,6 +810,8 @@ static int hmm_range_fault_locked(struct hmm_range > *range, bool *locked) > * output, and all >= are still at their input values. > */ > } while (ret == -EBUSY); > + > + hmm_vma_post_range_fault(&hmm_vma_walk); > return ret; > } [Severity: High] What happens in mm/hmm.c:hmm_range_fault_locked() when migration is enabled (as activated in the subsequent commit "mm: enable device page migration from HMM pagewalk") and a page in the range needs to be faulted in via hmm_do_fault()? On the first iteration of walk_page_range(), hmm_vma_walk_test() -> hmm_vma_capture_migrate_range() calls mmu_notifier_invalidate_range_start() and sets hmm_vma_walk->mmu_armed = true before any PTEs are inspected, while mmu_notifier_invalidate_range_end() is deferred until hmm_vma_post_range_fault() after the entire do { ... } while (ret == -EBUSY) loop completes. When walk_page_range() returns HMM_FAULT_PENDING, hmm_do_fault() -> handle_mm_fault() runs inside the active mmu_notifier_invalidate_range_start() and mmu_notifier_invalidate_range_end() section: 1. Across sleeping page faults (such as swap/file I/O, reclaim, userfaultfd, or migrate_to_ram()), mm->notifier_subscriptions->active_invalidate_ranges remains elevated and invalidate_seq remains odd, preventing mn_itree_inv_end() from completing nested invalidations and blocking concurrent mmu_interval_read_begin() and mmu_interval_notifier_remove() callers on subscriptions->wq. 2. When range->notifier is non-NULL, any nested invalidation inside handle_mm_fault() (for example, wp_page_copy() on a write fault or remove_device_exclusive_entry()) or any notifier not filtered by range->dev_private_owner updates range->notifier->invalidate_seq via mmu_interval_set_seq(). When hmm_do_fault() returns -EBUSY to re-walk the newly faulted page, the next iteration of the loop hits mmu_interval_check_retry(range->notifier, range->notifier_seq) and aborts with -EBUSY. 3. Because mmu_notifier_invalidate_range_start() is only called once before faulting when !hmm_vma_walk->mmu_armed, any PTE faulted into a previously empty entry by hmm_do_fault() is subsequently unmapped and replaced with a migration entry on the retry walk without a post-fault mmu_notifier_invalidate_range_start() notification. Should the mmu_notifier_invalidate_range_start() and mmu_notifier_invalidate_range_end() pair be scoped around the migration unmapping step rather than held across the hmm_do_fault() loop? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
