Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] Updating `range->gpu_mapped` outside `notifier_lock` creates a TOCTOU race with MMU notifiers, leading to GPU UAF. - [Critical] Fallible memory allocation inside the MMU notifier callback (`amdgpu_svm_range_zap_ptes`) can cause a GPU use-after-free. - [High] Missing `goto retry;` logic after `-EOPNOTSUPP` eviction in `amdgpu_svm_range_get_pages` permanently fails mapping. - [High] TLB is flushed and mapping is marked complete before asynchronous PDE updates finish. - [High] `amdgpu_svm_range_map_attrs` fails the ioctl with `-EAGAIN` upon MMU notifier invalidation instead of retrying. - [Medium] Ignored return value of `dma_fence_wait` in `amdgpu_svm_range_update_mapping` masks `-ERESTARTSYS` errors. --
commit fcda6b5ee02461678a8cd1586240f44fb136a85f Author: Honglei Huang <[email protected]> Subject: drm/amdgpu: implement SVM range GPU mapping core This patch implements GPU page table mapping in amdgpu_svm_range.c. It introduces functions for validating, finding, evicting, and updating SVM ranges, as well as handling PTE flags and updating mappings. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > new file mode 100644 > index 0000000000000..e1b392b2cf56a > --- /dev/null > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c [ ... ] > +int > +amdgpu_svm_range_zap_ptes(struct amdgpu_svm *svm, > + struct amdgpu_svm_range *range, > + unsigned long start_page, > + unsigned long last_page) > +{ > + struct dma_fence *fence = NULL; > + unsigned int flags; > + int ret; > + > + if (last_page < start_page) > + return 0; > + > + flags = memalloc_noreclaim_save(); > + ret = amdgpu_vm_update_range(svm->adev, svm->vm, false, true, true, > false, > + NULL, start_page, last_page, 0, 0, 0, NULL, > + NULL, &fence); > + memalloc_noreclaim_restore(flags); [Severity: Critical] Is it safe to rely on amdgpu_vm_update_range() inside the MMU notifier callback path? amdgpu_vm_update_range() dynamically allocates memory even when called within a memalloc_noreclaim_save() block (e.g., allocating tlb_cb). Under memory pressure, if this allocation fails, the PTEs will not be cleared. Since the MMU notifier cannot fail gracefully to prevent the physical page from being freed, could this result in the GPU retaining valid access to freed memory and causing a use-after-free? [ ... ] > +int amdgpu_svm_range_get_pages(struct amdgpu_svm *svm, > + struct drm_gpusvm_range *range, > + struct drm_gpusvm_ctx *ctx) > +{ [ ... ] > + if (ret == -EOPNOTSUPP) { > + AMDGPU_SVM_ERR( > + "range get pages -EOPNOTSUPP, evict and retry: > gpuva=[0x%lx-0x%lx) ret=%d\n", > + drm_gpusvm_range_start(range), > + drm_gpusvm_range_end(range), ret); > + amdgpu_svm_range_evict(svm, range); > + } > + > + return ret; > +} [Severity: High] Should there be a retry loop after the eviction? When drm_gpusvm_range_get_pages() returns -EOPNOTSUPP for unmappable device memory, this code successfully evicts it back to system RAM but then just falls through and returns -EOPNOTSUPP anyway. Does this lack of a "goto retry" statement permanently fail the mapping operation instead of recovering it? [ ... ] > +int amdgpu_svm_range_update_mapping(struct amdgpu_svm *svm, > + struct amdgpu_svm_range *range, > + const struct amdgpu_svm_attrs *attrs, > + bool read_only, > + bool intr, bool wait, > + bool flush_tlb) > +{ [ ... ] > + drm_gpusvm_notifier_unlock(&svm->gpusvm); > + memalloc_noreclaim_restore(flags); > + > + if (!ret && fence) > + dma_fence_wait(fence, intr); [Severity: Medium] Is it intentional to discard the return value of dma_fence_wait() here? If the wait is interrupted by a signal (since intr can be true), it returns -ERESTARTSYS. Because ret remains 0, the code proceeds as if the GPU mapping completed successfully. Could this lead to userspace scheduling GPU execution on memory whose page tables have not finished updating? > + dma_fence_put(fence); > + > + if (!ret) > + ret = amdgpu_vm_update_pdes(svm->adev, svm->vm, false); > + > + if (!ret) { > + if (flush_tlb) > + svm->flush_tlb(svm); [Severity: High] Should we wait for the asynchronous PDE updates to finish before returning? The amdgpu_vm_update_pdes() call uses immediate=false, meaning PDE updates are asynchronously queued to the SDMA ring. However, this function immediately flushes the CPU-side TLB and returns success to userspace. Can a user compute shader begin executing immediately and hit a GPU page fault because the hardware page directory entries have not yet been written? > + WRITE_ONCE(range->attr_flags, attrs->flags); > + WRITE_ONCE(range->gpu_mapped, true); [Severity: Critical] Could updating range->gpu_mapped outside the notifier_lock create a TOCTOU race with MMU notifiers? The lock is released before setting WRITE_ONCE(range->gpu_mapped, true). Concurrently, an MMU notifier could run on another CPU, acquire the notifier_lock, and check the flag. Because it hasn't been updated yet, the notifier sees false, skips zapping the hardware PTEs, and allows the CPU to free the physical memory. Should gpu_mapped be updated inside the notifier_lock critical section to prevent a GPU use-after-free? > + range->validate_timestamp = ktime_get_boottime(); > + } > + > + drm_exec_fini(&exec); > + return ret; > +} [ ... ] > +int > +amdgpu_svm_range_map_attrs(struct amdgpu_svm *svm, [ ... ] > + ret = amdgpu_svm_range_update_mapping(svm, range, attrs, > + map_ctx.read_only, > + true, true, > + true); > + if (ret) { > + AMDGPU_SVM_ERR("failed to update gpu map for range > [0x%lx-0x%lx), ret=%d\n", > + drm_gpusvm_range_start(&range->base), > + drm_gpusvm_range_end(&range->base), > ret); > + return ret; > + } [Severity: High] Should this handle -EAGAIN by retrying instead of propagating the error to userspace? If the MMU notifier sequence changes concurrently during mapping, amdgpu_svm_range_update_mapping() safely catches this and returns -EAGAIN. However, there is no retry loop here to handle that expected condition. Will normal concurrent memory reclaim cause spurious ioctl failures to userspace if we abort the entire ioctl instead of transparently retrying the get_pages() and locking sequence? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
