Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [Critical] drm/nouveau/dmem: freed VRAM page mapped on migrate_vma_pages() abort - [Critical] drm/nouveau/dmem: copy timeout in nouveau_dmem_migrate_fault() maps uncopied page - [Critical] drm/nouveau/svm: nouveau_svm_range_invalidate() ignores all driver migrations - [High] drm/nouveau/dmem: unchecked nouveau_fence_new() in nouveau_dmem_fence_poll() callers - [High] drm/nouveau/svm: unguarded __nouveau_pfns_map_locked() on empty pfn in nouveau_dmem_migrate_fault() - [High] drm/nouveau/dmem: unsynchronized access to migrate.chan in nouveau_dmem_migrate_fault() - [Medium] drm/nouveau: unguarded pfn map and retry state in nouveau_dmem_migrate_fault() - [Medium] drm/nouveau/svm: __nouveau_pfns_map_locked() drops nvif_object_ioctl() error - [Low] drm/nouveau/svm: missing kernel-parameters.txt entry for svm_migrate_on_fault --
commit 44e0adde7e1aeb837684fa123e764779afbd3db4 Author: Mika Penttilä <[email protected]> drm/nouveau/svm: add support for migrate on fault This commit adds migrate-on-fault support for Nouveau SVM using HMM_PFN_REQ_MIGRATE and migrate_hmm_range_setup(), gated by a new svm_migrate_on_fault module parameter. It introduces nouveau_dmem_migrate_fault() to fault and collect migratable pages in a single hmm_range_fault() walk, and polls migration fences via nouveau_dmem_fence_poll(). > diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c > b/drivers/gpu/drm/nouveau/nouveau_dmem.c > index e74d7bb975a86..998c63e2ae78b 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c [ ... ] > @@ -150,6 +155,32 @@ static void nouveau_dmem_fence_done(struct nouveau_fence > **fence) > * the hmem object. > */ > } > + > + /* 0 on completion, -EBUSY on timeout, or other errno; propagated. */ > + return ret; > +} > + > +/* > + * Poll the copy fence instead of the lazy (interrupt-driven) wait since > + * completion IRQ is randomly missed, hitting the timeout. > + * > + * Returns 0 on completion, -EBUSY on timeout. Consumes the fence reference. > + */ > +static int nouveau_dmem_fence_poll(struct nouveau_fence **fence) > +{ > + unsigned long deadline = jiffies + 30 * HZ; > + int ret = 0; > + > + while (!nouveau_fence_done(*fence)) { [Severity: High] What happens in nouveau_dmem_fence_poll() in drivers/gpu/drm/nouveau/nouveau_dmem.c when nouveau_fence_new() fails in its callers? In drivers/gpu/drm/nouveau/nouveau_fence.c, nouveau_fence_new() can fail in nouveau_fence_create() (without writing *pfence) or in nouveau_fence_emit() (which calls nouveau_fence_unref(pfence) and sets *pfence = NULL). Both callers of nouveau_dmem_fence_poll() ignore the return value of nouveau_fence_new(): 1. In nouveau_dmem_migrate_to_ram() in drivers/gpu/drm/nouveau/nouveau_dmem.c, fence is declared uninitialized on the stack: struct nouveau_fence *fence; ... nouveau_fence_new(&fence, dmem->migrate.chan); migrate_vma_pages(&args); fret = nouveau_dmem_fence_poll(&fence); 2. In nouveau_dmem_migrate_fault() in drivers/gpu/drm/nouveau/nouveau_dmem.c, fence is initialized to NULL: if (migrated) nouveau_fence_new(&fence, drm->dmem->migrate.chan); ... if (migrated) fret = nouveau_dmem_fence_poll(&fence); Since nouveau_fence_done() in drivers/gpu/drm/nouveau/nouveau_fence.c immediately dereferences fence via nouveau_fctx(fence) and dma_fence_is_signaled(&fence->base), won't a failure in nouveau_fence_new() cause an uninitialized or NULL pointer dereference in nouveau_dmem_fence_poll()? > + if (time_after(jiffies, deadline)) { > + ret = -EBUSY; > + break; > + } > + schedule_timeout_uninterruptible(msecs_to_jiffies(1)); > + } > + nouveau_fence_unref(fence); > + > + return ret; > } [ ... ] > @@ -266,9 +299,12 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct > vm_fault *vmf) > > nouveau_fence_new(&fence, dmem->migrate.chan); > migrate_vma_pages(&args); > - nouveau_dmem_fence_done(&fence); > + fret = nouveau_dmem_fence_poll(&fence); > dma_unmap_page(drm->dev->dev, dma_info.dma_addr, dma_info.size, > DMA_BIDIRECTIONAL); > + if (fret) > + /* Copy failed; the pte now points at possibly stale sysram > data. */ > + ret = VM_FAULT_SIGBUS; > done: > migrate_vma_finalize(&args); > err: [ ... ] > @@ -832,6 +884,215 @@ static void nouveau_dmem_migrate_chunk(struct > nouveau_drm *drm, [ ... ] > +int > +nouveau_dmem_migrate_fault(struct nouveau_drm *drm, struct nouveau_svmm > *svmm, > + struct mmu_interval_notifier *notifier, > + unsigned long start, unsigned long end, > + unsigned long hmm_flags, > + u64 *out_pfn0, unsigned long *out_addr, > + unsigned int *out_shift) > +{ [ ... ] > + /* hmm_range_fault() shares the source pfn array with migrate_vma. */ > + range.hmm_pfns = migrate.src; > + > + do { > + mmap_read_lock(mm); > + range.notifier_seq = mmu_interval_read_begin(notifier); > + > + ret = hmm_range_fault(&range); > + /* > + * Snapshot the pfn before migrate_hmm_range_setup() overwrites > the > + * shared array; used to map in place if nothing migrates. > + */ > + if (!ret) > + hmm_pfn0 = range.hmm_pfns[0]; > + migrate_hmm_range_setup(&range); [Severity: Medium] When the retry loop in nouveau_dmem_migrate_fault() in drivers/gpu/drm/nouveau/nouveau_dmem.c repeats after ret == -EBUSY (either from hmm_range_fault() or from mmu_interval_read_retry()), does migrate.src (aliased by range.hmm_pfns) retain stale migrate_pfn values from the previous iteration? migrate_hmm_range_setup() in mm/migrate_device.c overwrites range.hmm_pfns[i] with migrate_pfn(...) flags, and neither migrate_vma_pages() nor migrate_vma_finalize() clears migrate.src[]. Unlike migrate_vma_setup() in mm/migrate_device.c, which zeroes args->src before calling hmm_range_fault(), nouveau_dmem_migrate_fault() re-enters hmm_range_fault(&range) without clearing migrate.src. If the next walk hits an early return path that does not overwrite *hmm_pfn (for example, softleaf_is_migration() in hmm_vma_handle_pte()), won't range.hmm_pfns still hold stale migrate_pfn data from the prior attempt? > + if (ret) { > + migrate_vma_pages(&migrate); > + migrate_vma_finalize(&migrate); > + goto unlock; > + } > + > + n = nouveau_dmem_migrate_alloc_and_copy(drm, svmm, &migrate, > + dma_info, pfns, > + &nr_dma, &order); > + > + /* Only a migrated page has an in-flight CE copy to unblock > below. */ > + for (i = 0, migrated = false; i < npages; i++) { > + if (migrate.dst[i]) { > + migrated = true; > + break; > + } > + } > + > + /* > + * Track a copy fence only when a copy was enqueued; waiting on > an > + * empty fence would block until the fault's CTXSW_TIMEOUT. > + */ > + if (migrated) > + nouveau_fence_new(&fence, drm->dmem->migrate.chan); [Severity: High] How is access to drm->dmem->migrate.chan synchronized in nouveau_dmem_migrate_fault() in drivers/gpu/drm/nouveau/nouveau_dmem.c? Here, nouveau_dmem_migrate_alloc_and_copy() (which calls nvc0b5_migrate_copy() and nvc0b5_migrate_clear() to push multi-word DMA commands via PUSH_WAIT(push, ...) on drm->dmem->migrate.chan->chan.push) and nouveau_fence_new(&fence, drm->dmem->migrate.chan) run while holding only mmap_read_lock(mm). Because drm->dmem->migrate.chan aliases drm->ttm.chan (set up in nouveau_dmem_migrate_init()), can this race with concurrent CPU faults in nouveau_dmem_migrate_to_ram() (which pushes copy commands under svmm->mutex and calls nouveau_fence_new() outside svmm->mutex) or concurrent TTM buffer moves in nouveau_bo_move_m2mf() in drivers/gpu/drm/nouveau/nouveau_bo.c (which serializes using cli->mutex), corrupting the shared channel pushbuffer or racing on ++fctx->sequence? > + migrate_vma_pages(&migrate); > + > + /* > + * The fault-in path must replay the faulting channel before > waiting on > + * the copy fence: with the fault still unreplayed the copy > does not > + * make progress. GPU has no mapping so it might refault, until > it > + * gets the migrated or mirrored mapping. > + */ > + if (drm->svm && migrated) > + nouveau_svm_fault_replay(drm->svm); > + > + if (migrated) > + fret = nouveau_dmem_fence_poll(&fence); > + > + mutex_lock(&svmm->mutex); > + if (fret) { > + /* Copy did not complete; don't map a half-copied page. > */ > + ret = -EIO; [Severity: Critical] If nouveau_dmem_fence_poll(&fence) times out and returns -EBUSY here in nouveau_dmem_migrate_fault() (or above in nouveau_dmem_migrate_to_ram()) in drivers/gpu/drm/nouveau/nouveau_dmem.c, doesn't the page migration still get committed with uncopied data while DMA may still be active? Because migrate_vma_pages(&migrate) was already called before nouveau_dmem_fence_poll(&fence), the folio metadata migration has already succeeded and MIGRATE_PFN_MIGRATE remains set in migrate.src[0]. When fret is non-zero: 1. dma_unmap_page() is called immediately on the source page (or destination page in nouveau_dmem_migrate_to_ram()) even though the timed-out copy engine DMA may still be in flight. 2. migrate_vma_finalize(&migrate) sees MIGRATE_PFN_MIGRATE still set in migrate.src[0], so it installs a device-private swap PTE for the half-copied VRAM page into the CPU page tables and frees the valid source system RAM folio (or in nouveau_dmem_migrate_to_ram(), installs a present CPU PTE to the stale system RAM page and frees the source VRAM folio). 3. In the GPU fault path, nouveau_dmem_migrate_fault() returns -EIO to nouveau_range_fault_and_migrate(), and nouveau_svm_fault() in drivers/gpu/drm/nouveau/nouveau_svm.c then executes: ret = nouveau_range_fault_and_migrate(svmm, svm->drm, args, hmm_flags, ¬ifier); if (ret) ret = nouveau_range_fault(svmm, svm->drm, args, __struct_size(args), hmm_flags, ¬ifier); Won't nouveau_range_fault() walk the CPU page table, find the device-private swap PTE just installed by migrate_vma_finalize(), and map that half-copied VRAM page into the GPU anyway? > + } else if (mmu_interval_read_retry(notifier, > range.notifier_seq)) { > + ret = -EBUSY; > + } else { > + unsigned long map_addr = migrate.start; > + unsigned int page_shift = PAGE_SHIFT + order; > + > + /* > + * If nothing migrated might still be able to mirror. > + * In that case use the snapshot hmm_pfn. > + * npages == 1 always today. > + */ > + if (npages == 1 && !(pfns[0] & NVIF_VMM_PFNMAP_V0_V)) > + pfns[0] = nouveau_dmem_hmm_pfn_to_map(hmm_pfn0, > + start, &map_addr, &page_shift); > + > + __nouveau_pfns_map_locked(svmm, map_addr, pfns, n, > + page_shift); [Severity: Critical] Can nouveau_dmem_migrate_fault() in drivers/gpu/drm/nouveau/nouveau_dmem.c map a VRAM page into the GPU that migrate_vma_pages() rejected and that migrate_vma_finalize() is about to free? Before migrate_vma_pages(&migrate) is called, nouveau_dmem_migrate_alloc_and_copy() -> nouveau_dmem_migrate_copy_one() allocates a destination VRAM page dpage for any entry with MIGRATE_PFN_MIGRATE set and populates pfns[0]: *pfn = NVIF_VMM_PFNMAP_V0_V | NVIF_VMM_PFNMAP_V0_VRAM | ((paddr >> PAGE_SHIFT) << NVIF_VMM_PFNMAP_V0_ADDR_SHIFT); Next, migrate_vma_pages(&migrate) calls __migrate_device_pages() in mm/migrate_device.c. If migration of the folio is rejected or fails — which happens for all present file-backed or shared/shmem pages (collected by hmm_vma_handle_migrate_prepare(), then rejected by __migrate_device_pages() at !folio_test_anon(folio)), or when folio_free_swap() or folio_migrate_mapping() fails with -EAGAIN due to extra references — __migrate_device_pages() aborts the migration by clearing MIGRATE_PFN_MIGRATE in migrate.src[0]. However, pfns[0] still has NVIF_VMM_PFNMAP_V0_V set from nouveau_dmem_migrate_copy_one(). Because nouveau_dmem_migrate_fault() checks !(pfns[0] & NVIF_VMM_PFNMAP_V0_V) without checking whether migrate.src[0] & MIGRATE_PFN_MIGRATE is still set after migrate_vma_pages(): 1. The in-place mirror fallback is skipped. 2. __nouveau_pfns_map_locked() maps the aborted VRAM page dpage into the GPU VMM. 3. Immediately below, migrate_vma_finalize(&migrate) sees !(migrate.src[0] & MIGRATE_PFN_MIGRATE), restores the CPU PTE to the original system RAM folio, and calls folio_put(dst) on dpage, freeing it back to dmem->free_pages via nouveau_dmem_folio_free() while the GPU retains a live mapping to it. Should nouveau_dmem_migrate_fault() check migrate.src[i] & MIGRATE_PFN_MIGRATE after migrate_vma_pages(&migrate) and clear pfns[i] when migration did not succeed? > + > + *out_pfn0 = pfns[0]; > + *out_addr = map_addr; > + *out_shift = page_shift; > + } > + mutex_unlock(&svmm->mutex); > + > + if (fret) > + NV_ERROR(drm, > + "svm: migrate copy did not complete (%d) for > %lx-%lx\n", > + fret, start, end); > + > + while (nr_dma--) { > + dma_unmap_page(drm->dev->dev, dma_info[nr_dma].dma_addr, > + dma_info[nr_dma].size, > DMA_BIDIRECTIONAL); > + } > + migrate_vma_finalize(&migrate); > +unlock: > + mmap_read_unlock(mm); > + } while (ret == -EBUSY && !time_after(jiffies, timeout)); > + > + /* Nothing migrated or mappable in place, fallback to > nouveau_range_fault(). */ > + if (!ret && !(pfns[0] & NVIF_VMM_PFNMAP_V0_V)) > + ret = -ENOENT; [Severity: High] Why is __nouveau_pfns_map_locked() called before checking whether pfns[0] & NVIF_VMM_PFNMAP_V0_V is set in nouveau_dmem_migrate_fault() in drivers/gpu/drm/nouveau/nouveau_dmem.c? hmm_range_fault(&range) can return 0 while hmm_pfn0 does not have HMM_PFN_VALID set — for example: - when faulting on a PMD-mapped THP or PTE-mapped large folio where folio_trylock() or folio splitting fails in hmm_vma_handle_migrate_prepare_pmd() or hmm_vma_walk_pmd() in mm/hmm.c (setting HMM_PFN_ERROR and returning 0), - when faulting on the huge zero folio in hmm_vma_handle_migrate_prepare_pmd(), or - on a read fault to the shared zero page where hmm_vma_handle_migrate_prepare() sets *hmm_pfn = HMM_PFN_MIGRATE (without HMM_PFN_VALID) and VRAM allocation in nouveau_dmem_migrate_copy_one() fails. In those cases, nothing is migrated (pfns[0] is NVIF_VMM_PFNMAP_V0_NONE) and nouveau_dmem_hmm_pfn_to_map(hmm_pfn0, ...) also returns NVIF_VMM_PFNMAP_V0_NONE with page_shift = PAGE_SHIFT. Calling __nouveau_pfns_map_locked() with pfns[0] == NVIF_VMM_PFNMAP_V0_NONE invokes nvkm_vmm_pfn_map() in drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c with map == false at 4KB granularity. If the faulting address was already mapped in the GPU VMM (for instance, mapped read-only prior to a write fault), nvkm_vmm_pfn_map() splits the GPU VMA and unmaps the 4KB range; and if the existing GPU VMM mapping was a 2MB page (vma->refd at shift 21 with no 4KB page table allocated), walking down to shift 12 with ref == false in nvkm_vmm_iter() leaves it.pt[0] == NULL and dereferences it in nvkm_vmm_unref_ptes(). Should __nouveau_pfns_map_locked() only be called when pfns[0] & NVIF_VMM_PFNMAP_V0_V is set? > + > + nouveau_pfns_free(pfns); > +out_free_dma: > + kfree(dma_info); > +out_free_dst: > + kfree(migrate.dst); > +out_free_src: > + kfree(migrate.src); > + return ret; > +} [ ... ] > diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c > b/drivers/gpu/drm/nouveau/nouveau_svm.c > index 58735446d7832..83ea480abcf58 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_svm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c [ ... ] > @@ -78,6 +79,13 @@ struct nouveau_svm { > #define SVM_DBG(s,f,a...) NV_DEBUG((s)->drm, "svm: "f"\n", ##a) > #define SVM_ERR(s,f,a...) NV_WARN((s)->drm, "svm: "f"\n", ##a) > > +/* When set, migrate the faulting range to VRAM on GPU fault (default off). > */ > +static bool nouveau_svm_migrate_on_fault; > +module_param_named(svm_migrate_on_fault, nouveau_svm_migrate_on_fault, bool, > + 0644); > +MODULE_PARM_DESC(svm_migrate_on_fault, > + "migrate system memory to VRAM on GPU fault (default off)"); [Severity: Low] This adds the nouveau.svm_migrate_on_fault module/boot parameter in drivers/gpu/drm/nouveau/nouveau_svm.c. Should this parameter also be documented in Documentation/admin-guide/kernel-parameters.txt? [ ... ] > @@ -514,6 +523,14 @@ static bool nouveau_svm_range_invalidate(struct > mmu_interval_notifier *mni, > range->owner == sn->svmm->vmm->cli->drm->dev) > return true; > > + /* > + * Ignore our own migrate-on-fault invalidation; otherwise its > + * MMU_NOTIFY_MIGRATE bumps the seq and read_retry() spins to timeout. > + */ > + if (range->event == MMU_NOTIFY_MIGRATE && > + range->owner == sn->svmm->vmm->cli->drm->dev) > + return true; [Severity: Critical] Doesn't ignoring all MMU_NOTIFY_MIGRATE events with range->owner == sn->svmm->vmm->cli->drm->dev in nouveau_svm_range_invalidate() in drivers/gpu/drm/nouveau/nouveau_svm.c also ignore concurrent migrations triggered by other paths on the same device, allowing freed VRAM or system RAM pages to be mapped into the GPU VMM? drm->dev is also passed as pgmap_owner in nouveau_dmem_migrate_to_ram() (on CPU page faults migrating device-private VRAM pages back to system RAM) and in nouveau_dmem_migrate_vma() (on nouveau_svmm_bind() migrating system RAM to VRAM). Consider two races: 1. A page is currently in device-private VRAM. The GPU faults on it (either via nouveau_range_fault() when svm_migrate_on_fault is off or on FAULT_ACCESS_PREFETCH, or via nouveau_dmem_migrate_fault() which only sets MIGRATE_VMA_SELECT_SYSTEM so hmm_vma_handle_migrate_prepare() skips the device-private page without locking or referencing it). HMM records the VRAM page into range.hmm_pfns[0] / hmm_pfn0. Concurrently under shared mmap_read_lock(mm), a CPU fault on the same address runs nouveau_dmem_migrate_to_ram(). Its migrate_vma_setup() emits MMU_NOTIFY_MIGRATE with owner == drm->dev, which nouveau_svm_range_invalidate() now ignores without taking svmm->mutex or calling mmu_interval_set_seq(). nouveau_dmem_migrate_to_ram() invalidates the GPU VMM under svmm->mutex, copies the page to RAM, and frees the VRAM folio in migrate_vma_finalize(). The GPU fault worker then takes svmm->mutex, sees mmu_interval_read_retry() == false because invalidate_seq was never bumped, and maps the freed VRAM page into the GPU VMM. 2. In nouveau_range_fault(), after hmm_range_fault_unlocked_timeout() reads a system RAM PFN and drops mmap_read_lock(mm), a concurrent nouveau_dmem_migrate_vma() or nouveau_dmem_migrate_fault() unmaps the single-mapped PTE in hmm_vma_handle_migrate_prepare() while emitting only MMU_NOTIFY_MIGRATE with owner == drm->dev, migrates the page to VRAM, and frees the system RAM folio. Because nouveau_svm_range_invalidate() ignored that MMU_NOTIFY_MIGRATE event, nouveau_range_fault() sees mmu_interval_read_retry() == false and maps the freed system RAM page into the GPU VMM. [ ... ] > @@ -922,21 +965,29 @@ nouveau_pfns_free(u64 *pfns) > } > > void > -nouveau_pfns_map(struct nouveau_svmm *svmm, struct mm_struct *mm, > - unsigned long addr, u64 *pfns, unsigned long npages, > - unsigned int page_shift) > +__nouveau_pfns_map_locked(struct nouveau_svmm *svmm, unsigned long addr, > + u64 *pfns, unsigned long npages, > + unsigned int page_shift) > { > struct nouveau_pfnmap_args *args = nouveau_pfns_to_args(pfns); > > + lockdep_assert_held(&svmm->mutex); > + > args->p.addr = addr; > args->p.size = npages << page_shift; > args->p.page = page_shift; > > - mutex_lock(&svmm->mutex); > - > nvif_object_ioctl(&svmm->vmm->vmm.object, args, > struct_size(args, p.phys, npages), NULL); > +} [Severity: Medium] Should __nouveau_pfns_map_locked() in drivers/gpu/drm/nouveau/nouveau_svm.c propagate the return value of nvif_object_ioctl() to nouveau_dmem_migrate_fault()? In nouveau_range_fault() and nouveau_atomic_range_fault(), the return value of nvif_object_ioctl() is returned to nouveau_svm_fault() so that failed mappings trigger nouveau_svm_fault_cancel_fault(). In contrast, __nouveau_pfns_map_locked() discards the return value of nvif_object_ioctl(), and nouveau_dmem_migrate_fault() only checks !(pfns[0] & NVIF_VMM_PFNMAP_V0_V). If nvkm_vmm_pfn_map() in drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c returns early with -EINVAL (for example, when hmm_pfn_to_map_order() returns an order such as a 1GiB PUD or 64KiB hugetlb page that the GPU VMM page table descriptor does not support) or -ENOENT (when the address is not in a managed GPU VMA), nvkm_vmm_pfn_map() returns before the loop that clears NVKM_VMM_PFN_V in pfns[0]. Won't nouveau_dmem_migrate_fault() then falsely return 0, causing nouveau_svm_fault() to replay the unhandled GPU fault in an infinite fault loop instead of cancelling it? > > +void > +nouveau_pfns_map(struct nouveau_svmm *svmm, struct mm_struct *mm, > + unsigned long addr, u64 *pfns, unsigned long npages, > + unsigned int page_shift) > +{ > + mutex_lock(&svmm->mutex); > + __nouveau_pfns_map_locked(svmm, addr, pfns, npages, page_shift); > mutex_unlock(&svmm->mutex); > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
