Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] mm/hmm: memset() in hmm_range_fault_locked() wipes collected pfns on retry - [Low] mm/hmm: build failure in hmm_range_fault_locked() when !CONFIG_MIGRATION --
commit 9e255c525419b859ce3c5e39d7de2c6153874013 Author: Mika Penttilä <[email protected]> mm/hmm: preparations for HMM to participate in migration This commit prepares hmm_range_fault() to participate in page migration by capturing the VMA during page table walks, managing MMU_NOTIFY_MIGRATE invalidations, and clearing range->hmm_pfns at the start of migrating faults. > diff --git a/mm/hmm.c b/mm/hmm.c > index 2f1e98c6b6440..9220e6648636d 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -694,19 +774,45 @@ 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; > + > + /* > + * Migration takes its requests only from default_flags, so the input > + * pfns carry nothing. Clear them so that collected entries can be > + * told apart reliably during the walk and on rollback. > + */ > + if (hmm_select_migrate(range)) { > + WARN_ON_ONCE(range->pfn_flags_mask); > + WARN_ON_ONCE(range->dev_private_owner != > + range->migrate->pgmap_owner); [Severity: Low] Will this break the build when CONFIG_HMM_MIRROR=y and CONFIG_MIGRATION=n? Several Kconfig options (such as CONFIG_INFINIBAND_ON_DEMAND_PAGING and CONFIG_DRM_AMDGPU_USERPTR) select CONFIG_HMM_MIRROR without depending on CONFIG_MIGRATION. In include/linux/hmm.h, struct migrate_vma is only forward-declared, and in include/linux/migrate.h, its definition is guarded by #ifdef CONFIG_MIGRATION while a stub is provided for hmm_select_migrate() when !CONFIG_MIGRATION. Even though hmm_select_migrate(range) returns 0 when CONFIG_MIGRATION is disabled, the compiler still type-checks range->migrate->pgmap_owner inside hmm_range_fault_locked() in mm/hmm.c and fails due to dereferencing a pointer to an incomplete type. Should this check be moved into a helper in include/linux/migrate.h with a matching !CONFIG_MIGRATION stub? > + memset(range->hmm_pfns, 0, > + ((range->end - range->start) >> PAGE_SHIFT) * > + sizeof(*range->hmm_pfns)); > + } > + > 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; > + } [Severity: High] Can unconditionally calling memset() on range->hmm_pfns in hmm_range_fault_locked() cause a self-deadlock on the folio lock or leak locked folios when a migrating hmm_range_fault() call is retried after -EBUSY? When migration is enabled in this series with range->notifier set (such as in dmirror_range_fault() in lib/test_hmm.c), walk_page_range() collects present pages by locking each folio, elevating its reference count, installing a migration PTE, and marking range->hmm_pfns[i] with HMM_PFN_VALID | HMM_PFN_MIGRATE. If a later page in the range needs a fault, walk_page_range() returns HMM_FAULT_PENDING, hmm_vma_migrate_disarm() closes the MMU_NOTIFY_MIGRATE invalidation, and hmm_do_fault() calls handle_mm_fault() while the previously collected folios stay locked by current and mapped as migration PTEs. If handle_mm_fault() or a concurrent operation triggers an MMU notifier invalidation, mmu_interval_check_retry() on the next loop iteration returns true and hmm_range_fault_locked() exits with -EBUSY (or the caller retries after mmu_interval_read_retry() fails). When the caller retries hmm_range_fault(range) with the same range->hmm_pfns array, the unconditional memset(range->hmm_pfns, 0, ...) at the top of hmm_range_fault_locked() wipes the HMM_PFN_VALID | HMM_PFN_MIGRATE markers for the folios that are still locked and installed as migration PTEs. On the retry walk, hmm_pfn_collected() will return false for those entries, causing hmm_vma_handle_pte() or hmm_vma_walk_pmd() to treat current's own migration entries as another task's migration entries and wait in migration_entry_wait() on a folio already locked by current (or leak the locked folios and migration PTEs if the caller aborts). -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
