Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] Partial MMU notifier unmaps zap a subset of GPU PTEs but drop all backing pages or disable PTE zapping for the rest of the range, leading to stale GPU page tables and use-after-free vulnerabilities. - [High] Concurrent MMU notifier and GC worker operations cause a double `list_add_tail()` on `svm->gc.list`, leading to linked list corruption. - [High] Self-deadlock in `amdgpu_svm_range_invalidate_interval` due to passing `unlocked=true` to `amdgpu_vm_update_range` while the VM PD lock is held. - [High] `dma_fence_wait` inside the MMU notifier path is called outside of the `memalloc_noreclaim` scope, causing fs_reclaim lockdep splats. - [High] DMA pages are unmapped before the GPU TLB is flushed during range invalidation, risking memory corruption or IOMMU faults. - [High] The GPU TLB flush is entirely skipped if an error occurs during page table zapping, leaving stale mappings active. --
commit 4b7d05253ea2ed3fdf73e5a506fb682970d5dd2a Author: Honglei Huang <[email protected]> Subject: drm/amdgpu: implement SVM range notifier and GC helpers This commit adds MMU notifier event handling and garbage collection infrastructure for the GPU SVM implementation. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > index e1b392b2cf56a..57bcbaf0fdd2b 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c [ ... ] > +bool > +amdgpu_svm_range_notifier_event_begin(struct amdgpu_svm *svm, > + struct drm_gpusvm_range *range, > + const struct mmu_notifier_range > *mmu_range) > +{ > + struct amdgpu_svm_range *svm_range = to_amdgpu_svm_range(range); > + unsigned long start_page, last_page; > + > + amdgpu_svm_assert_in_notifier(svm); > + > + AMDGPU_SVM_RANGE_DEBUG(svm_range, "NOTIFIER"); > + > + if (range->pages.flags.unmapped || !svm_range->gpu_mapped) > + return false; > + > + AMDGPU_SVM_RANGE_DEBUG(svm_range, "NOTIFIER - EXECUTE"); > + > + start_page = max(drm_gpusvm_range_start(range), > + mmu_range->start) >> PAGE_SHIFT; > + last_page = (min(drm_gpusvm_range_end(range), > + mmu_range->end) >> PAGE_SHIFT) - 1; > + > + amdgpu_svm_range_zap_ptes(svm, svm_range, start_page, last_page); [Severity: High] Does calling amdgpu_svm_range_zap_ptes() here risk deadlocks? This executes in memory reclaim (fs_reclaim) context, but looking at amdgpu_svm_range_zap_ptes(), the memalloc_noreclaim flags are restored before waiting on a DMA fence: amdgpu_svm_range_zap_ptes() { ... memalloc_noreclaim_restore(flags); if (!ret && fence) { ret = dma_fence_wait(fence, false); ... } Waiting on a DMA fence while in memory reclaim context violates locking rules since the fence signaling may depend on other memory allocations. Could this trigger lockdep warnings and potential deadlocks? > + amdgpu_svm_range_invalidate_gpu_mapping(svm_range); > + > + return true; > +} [Severity: High] When an MMU notifier reports a partial unmap, the code above zaps only the affected subset of PTEs, but marks the entire range as unmapped. Does this leave stale GPU page tables for the unzapped portion? Because amdgpu_svm_range_notifier_event_end() unconditionally unmaps the DMA IOVAs for the entire range, the GPU retains valid PTEs pointing to DMA addresses that have now been unmapped from the IOMMU. If the OS reallocates these pages, could the GPU access them via the stale PTEs and cause IOMMU faults or use-after-free conditions? [ ... ] > +static void > +amdgpu_svm_gc_enqueue(struct amdgpu_svm *svm, > + struct amdgpu_svm_range *range, > + unsigned long start_page, unsigned long last_page) > +{ > + if (atomic_read(&svm->exiting)) > + return; > + > + spin_lock(&svm->work_lock); > + if (range->queue_state == AMDGPU_SVM_RANGE_NOT_QUEUED) { > + drm_gpusvm_range_get(&range->base); > + range->queue_state = AMDGPU_SVM_RANGE_IN_GC; > + } > + > + range->pending_start_page = min(range->pending_start_page, start_page); > + range->pending_last_page = max(range->pending_last_page, last_page); > + if (range->pending_ops == AMDGPU_SVM_RANGE_OP_NONE) > + list_add_tail(&range->work_node, &svm->gc.list); [Severity: High] Can concurrent MMU notifier and GC worker operations cause a double list addition on svm->gc.list? If the GC worker removes a range from gc.list via amdgpu_svm_range_dequeue_locked(), it sets queue_state to AMDGPU_SVM_RANGE_PROCESSING and pending_ops to AMDGPU_SVM_RANGE_OP_NONE. Concurrently, an MMU notifier could trigger amdgpu_svm_gc_enqueue(). Because pending_ops is AMDGPU_SVM_RANGE_OP_NONE, it executes list_add_tail() to re-add the range to gc.list and sets the UNMAP flag below, but leaves queue_state as AMDGPU_SVM_RANGE_PROCESSING. When the GC worker finishes, it calls amdgpu_svm_range_put_if_dequeued(), observes queue_state is AMDGPU_SVM_RANGE_PROCESSING and the UNMAP flag is set, and executes list_add_tail() again on the exact same node. Could this corrupt the linked list? > + range->pending_ops |= AMDGPU_SVM_RANGE_OP_UNMAP; > + spin_unlock(&svm->work_lock); > + > + queue_work(svm->gc.wq, &svm->gc.work); > +} [ ... ] > +int > +amdgpu_svm_range_invalidate_interval(struct amdgpu_svm *svm, > + unsigned long start_page, > + unsigned long last_page) > +{ > + unsigned long start = start_page << PAGE_SHIFT; > + unsigned long end = (last_page + 1) << PAGE_SHIFT; > + struct drm_gpusvm_notifier *notifier, *next_notifier; > + struct drm_gpusvm_ctx ctx = { .in_notifier = false }; > + struct drm_exec exec; > + bool needs_flush = false; > + int ret; > + > + amdgpu_svm_assert_locked(svm); > + > + ret = amdgpu_svm_range_lock_vm_pd(svm, &exec, true); > + if (ret) > + return ret; [Severity: High] Does holding the VM root PD lock here cause a self-deadlock when calling amdgpu_svm_range_zap_ptes() below? amdgpu_svm_range_invalidate_interval() acquires the VM root PD lock using the exec context. Later in the loop, it calls amdgpu_svm_range_zap_ptes(), which unconditionally passes unlocked=true to amdgpu_vm_update_range(): amdgpu_svm_range_zap_ptes() { ... ret = amdgpu_vm_update_range(svm->adev, svm->vm, false, true, true, ... } When unlocked=true is passed, amdgpu_vm_update_range() attempts to acquire the vm->root.bo lock using a new drm_exec context. Since the lock is already held by this thread under a different context, will this cause a deadlock or trigger a ww_mutex warning? > + > + drm_gpusvm_for_each_notifier_safe(notifier, next_notifier, &svm->gpusvm, > + start, end) { > + struct drm_gpusvm_range *range, *next_range; > + > + drm_gpusvm_for_each_range_safe(range, next_range, notifier, > + start, end) { > + struct amdgpu_svm_range *svm_range = > to_amdgpu_svm_range(range); > + bool crosses_boundary = start > > drm_gpusvm_range_start(range) || > + end < > drm_gpusvm_range_end(range); > + > + if (svm_range->gpu_mapped) { > + AMDGPU_SVM_RANGE_DEBUG(svm_range, > + crosses_boundary ? "ATTR DESTROY" : > + "ATTR ZAP PTE"); > + > + ret = amdgpu_svm_range_zap_ptes(svm, svm_range, > + drm_gpusvm_range_start(range) > >> PAGE_SHIFT, > + (drm_gpusvm_range_end(range) >> > PAGE_SHIFT) - 1); > + if (ret < 0) { > + drm_exec_fini(&exec); > + return ret; > + } [Severity: High] If amdgpu_svm_range_zap_ptes() fails, is it safe to return immediately without flushing the TLB? If previous ranges in the loop successfully zapped their PTEs, or if the current range was partially updated before the failure, skipping the svm->flush_tlb(svm) call leaves the GPU TLB out of sync with the page tables. Could these stale TLB entries allow unauthorized memory accesses? > + > + > amdgpu_svm_range_invalidate_gpu_mapping(svm_range); > + needs_flush = true; > + } > + > + if (crosses_boundary) { > + /* remove ranges crossing the boundary so GPU > fault > + * creates new ranges bounded by the updated > + * attr_range boundaries. > + * Evict devmem-backed pages back to sysmem > first > + * so VRAM-resident data is not lost when the > range > + * is destroyed. No-op for sysmem-only ranges. > + */ > + drm_gpusvm_range_get(range); > + > + amdgpu_svm_range_evict(svm, range); > + amdgpu_svm_range_remove(svm, svm_range, &ctx); [Severity: High] Does unmapping DMA pages here risk memory corruption or IOMMU faults? amdgpu_svm_range_remove() delegates to drm_gpusvm_range_unmap_pages(), which unmaps the physical pages from the GPU's DMA address space. However, the TLB flush (svm->flush_tlb(svm)) is deferred until after the loop completes. Between the DMA unmapping and the TLB flush, the GPU retains cached translations to the unmapped physical pages. If the OS reallocates these pages, could the GPU silently corrupt the newly allocated memory? > + > + amdgpu_svm_range_unqueue(svm, svm_range); > + > + drm_gpusvm_range_put(range); > + } > + } > + } > + > + drm_exec_fini(&exec); > + > + if (needs_flush) > + svm->flush_tlb(svm); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
