Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] mm: test_hmm: UAF due to concurrent unmaps during migration --
commit 561fbddf4a910a84fca4e2648f745dbccb247398 Author: Mika Penttilä <[email protected]> lib/test_hmm: add a new testcase for the migrate on fault This commit enhances the hmm test driver (lib/test_hmm) with a new test case for migrate on fault. It adds a new ioctl and test code to fault and migrate anonymous memory to device private memory. > diff --git a/lib/test_hmm.c b/lib/test_hmm.c > index cd88e8177d0ab..b39522c05d832 100644 > --- a/lib/test_hmm.c > +++ b/lib/test_hmm.c [ ... ] > @@ -382,7 +390,9 @@ static int dmirror_range_fault(struct dmirror *dmirror, > break; > } > > - ret = dmirror_do_fault(dmirror, range); > + /* update device page table after migration */ > + if (!migrate) > + ret = dmirror_do_fault(dmirror, range); > > mutex_unlock(&dmirror->mutex); > out: [Severity: High] Does skipping the page table update here leave us vulnerable to concurrent unmaps during migration? When migrate is true, the dmirror->mutex is dropped here without verifying the final sequence or updating the device page table. This opens a race window. [ ... ] > @@ -1377,6 +1387,114 @@ static int dmirror_migrate_to_device(struct dmirror > *dmirror, > return ret; > } > > +static int do_fault_and_migrate(struct dmirror *dmirror, struct hmm_range > *range) > +{ > + struct migrate_vma *migrate = range->migrate; > + int ret; > + > + mmap_read_lock(dmirror->notifier.mm); > + > + /* Fault-in pages for migration */ > + ret = dmirror_range_fault(dmirror, range); [ ... ] > + pr_debug("Migrating from sys mem to device mem\n"); > + > + dmirror_migrate_alloc_and_copy(migrate, dmirror); > + migrate_vma_pages(migrate); > + dmirror_migrate_finalize_and_map(migrate, dmirror); > + migrate_vma_finalize(migrate); [Severity: High] Can this sequence lead to a use-after-free of device pages if a concurrent unmap occurs after we dropped the mutex in dmirror_range_fault()? While mmap_read_lock() is held here, a concurrent madvise(MADV_DONTNEED) could still execute (since it only requires the read lock or per-VMA lock) and clear the newly installed migration PTEs, triggering the MMU interval notifier. Since the interval notifier invalidation would see an empty dmirror->pt (as we haven't mapped the pages yet), it would take no action. Then, dmirror_migrate_finalize_and_map() inserts the newly allocated device pages into dmirror->pt blindly, without re-checking mmu_interval_read_retry(). When migrate_vma_finalize() runs, it would fail to map the device page into the CPU page table because the migration PTE is gone, dropping the final reference and freeing the page while it is still mapped in the device page table. > +out: > + mmap_read_unlock(dmirror->notifier.mm); > + return ret; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
