Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mm: hmm: migration range start address truncated on page walk retry - [High] mm: hmm: unbalanced MMU notifier invalidation when dev_private_owner is NULL - [High] mm: hmm: NULL pointer dereference in hmm_range_fault_unlocked_timeout --
commit 7ddb4b8635599a4339659239ade3b1bbc78ceec7 Author: Mika Penttilä <[email protected]> mm/hmm: preparations for HMM to participate in migration For migration to happen after hmm_range_fault(), the vma has to retrieved. Luckily, pagewalk already resolves that for us, so just have to save it for further use. For migration, mmap_lock has to be hold for the whole operation, so vma stays stable. > diff --git a/mm/hmm.c b/mm/hmm.c > index 2f1e98c6b6440..be39d2e5403ab 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -400,6 +407,57 @@ 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; > + hmm_vma_walk->start = start; [Severity: High] Is the tracked start address unconditionally overwritten on every walk restart? If the walk requires a retry, returning -EBUSY, the outer loop restarts walk_page_range() from the intermediate faulting address. On retry, could this clobber the tracking start with the intermediate address, leading to a truncated start address? > + hmm_vma_walk->end = end; > + > + if (end - start > range->end - range->start) > + return -ERANGE; > + > + if (!hmm_vma_walk->mmu_range.owner) { [Severity: High] Could using the owner field as an initialization flag be problematic here? If range->dev_private_owner is legitimately NULL, mmu_range.owner becomes NULL. If a page fault retry occurs, the check passes again, causing a duplicate mmu_notifier_invalidate_range_start(). Can this cause unbalanced MMU notifier invalidation? > + 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); > + } > + > + 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_range.owner) { [Severity: High] Are we missing a call to mmu_notifier_invalidate_range_end() if the owner is NULL? Assuming a NULL owner means the invalidation was never started could permanently leak the invalidation if dev_private_owner is legitimately NULL. > + /* > + * 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, [Severity: High] Will this propagate the truncated start address back to the caller's migration struct? If the walk was retried and the start address was clobbered in hmm_vma_capture_migrate_range(), it could cause the migration setup to miscalculate the page count and read from the wrong array offset. > + hmm_vma_walk->end); > + } > + mmu_notifier_invalidate_range_end(&hmm_vma_walk->mmu_range); > + } > +} [ ... ] > @@ -694,19 +757,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); [Severity: High] How does this API change affect hmm_range_fault_unlocked_timeout()? The patch modifies the HMM API to allow range->notifier to be NULL for migration entry points, handling it gracefully here with hmm_range_fault_mm(). However, looking at hmm_range_fault_unlocked_timeout() in mm/hmm.c: int hmm_range_fault_unlocked_timeout(struct hmm_range *range, unsigned long timeout) { struct mm_struct *mm = range->notifier->mm; It unconditionally dereferences range->notifier->mm at its very first line. Can this trigger a NULL pointer dereference if driver code passes a migration range with a NULL notifier to the unlocked variant? > int ret; > > + /* Migration is incompatible with mmap lock drop */ > + if (locked && hmm_select_migrate(range)) > + return -EINVAL; > + -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
