Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues: - [High] Unprotected lazy initialization of SVM in `amdgpu_gem_svm_ioctl` allows concurrent threads to leak memory and duplicate MMU notifiers. - [High] `amdgpu_svm_handle_fault` passes a Page Frame Number (PFN) instead of a byte address to `amdgpu_gmc_filter_faults_remove`, breaking fault filtering. - [High] Concurrent SVM lookup by PASID during VM teardown can lead to a use-after-free of the amdgpu_svm context. - [High] Failure path in `amdgpu_svm_cache_init` leaves a dangling pointer leading to a Use-After-Free on subsequent calls. - [Medium] The commit message claims to add KBUILD_EXTRA_SYMBOLS for drm Module.symvers, but this is missing from the Makefile. - [Medium] Missing module unload cleanup for `amdgpu_svm_range_cache` and `amdgpu_svm_attr_range_cache`. - [Low] CONFIG_DRM_AMDGPU_SVM is a boolean option that selects CONFIG_DRM_GPUSVM (a tristate), causing unmet direct dependencies and link errors when DRM is built as a module. - [Low] The custom `clean-svm` phony target in the AMDGPU Makefile bypasses standard Kbuild conventions. Pre-existing issues: - [High] Preexisting bug: `svm_range_restore_pages` passes a PFN instead of a byte address to `amdgpu_gmc_filter_faults_remove`. -- commit 639687153f86cdb4a60c3608d22867b66fce3a14 Author: Honglei Huang <[email protected]> drm/amdgpu: integrate SVM into build system and VM fault path This commit wires up the AMDGPU SVM subsystem, including adding Kconfig options, Makefile rules, registering the ioctl, and routing GPU page faults to the new SVM handler. > Makefile: > - Build amdgpu_svm.o, amdgpu_svm_attr.o, amdgpu_svm_fault.o, > amdgpu_svm_range.o when CONFIG_DRM_AMDGPU_SVM=y > - KBUILD_EXTRA_SYMBOLS for drm Module.symvers [Severity: Medium] The commit message mentions adding KBUILD_EXTRA_SYMBOLS, but this doesn't appear in the actual Makefile changes. Was this accidentally omitted, potentially causing unresolved symbol warnings during out-of-tree builds? > diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig > b/drivers/gpu/drm/amd/amdgpu/Kconfig > index 12e4a41bf1f04..68f519b3479db 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Kconfig > +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig > @@ -81,6 +81,16 @@ config DRM_AMDGPU_USERPTR > This option selects CONFIG_HMM and CONFIG_HMM_MIRROR if it > isn't already selected to enabled full userptr support. > > +config DRM_AMDGPU_SVM > + bool "Enable AMDGPU SVM support (experimental)" > + depends on DRM_AMDGPU > + depends on DEVICE_PRIVATE > + select DRM_GPUSVM [Severity: Low] Does selecting a tristate (DRM_GPUSVM) from a boolean (DRM_AMDGPU_SVM) break dependency inheritance? If DRM is built as a module, this will force DRM_GPUSVM to be built-in, leading to Kconfig unmet dependency warnings and vmlinux link errors since the core DRM symbols won't be built-in. > + select HMM_MIRROR > + select MMU_NOTIFIER > + help > + Experimental SVM support based on DRM GPUSVM. > + > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile > b/drivers/gpu/drm/amd/amdgpu/Makefile > index ba80542ead9d9..884cdc5c1e4af 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > @@ -322,6 +322,15 @@ amdgpu-$(CONFIG_VGA_SWITCHEROO) += amdgpu_atpx_handler.o > amdgpu-$(CONFIG_ACPI) += amdgpu_acpi.o > amdgpu-$(CONFIG_HMM_MIRROR) += amdgpu_hmm.o > > +# svm support > +amdgpu-$(CONFIG_DRM_AMDGPU_SVM) += amdgpu_svm.o amdgpu_svm_attr.o \ > + amdgpu_svm_fault.o amdgpu_svm_range.o [Severity: Medium] Since we are integrating the SVM subsystem here, does the module exit hook (amdgpu_exit) properly clean up the kmem caches created in amdgpu_svm_cache_init()? It looks like amdgpu_svm_range_cache and amdgpu_svm_attr_range_cache might be permanently leaked on module unload because amdgpu_svm_cache_fini() is never called. [Severity: High] Also regarding amdgpu_svm_cache_init(), if amdgpu_svm_attr_cache_init() fails, the error path destroys amdgpu_svm_range_cache but doesn't set its pointer to NULL: free_out: amdgpu_svm_attr_cache_fini(); AMDGPU_SVM_KMEM_CACHE_DESTROY(amdgpu_svm_range_cache); // missing: amdgpu_svm_range_cache = NULL; Could this leave a dangling pointer that causes a use-after-free or panic on subsequent initialization attempts, since the check if (amdgpu_svm_range_cache) would incorrectly succeed? > + > +.PHONY: clean-svm > +clean-svm: > + rm -f $(obj)/amdgpu_svm.o $(obj)/amdgpu_svm_attr.o > $(obj)/amdgpu_svm_fault.o $(obj)/amdgpu_svm_range.o \ > + $(obj)/.amdgpu_svm.o.cmd $(obj)/.amdgpu_svm_attr.o.cmd > $(obj)/.amdgpu_svm_fault.o.cmd $(obj)/.amdgpu_svm_range.o.cmd [Severity: Low] Is this custom clean target necessary? Kbuild should automatically manage the cleanup of *.o and .*.cmd files during the standard make clean phase. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > index 503bb64c1e55f..46a074caba53a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c [ ... ] > @@ -3077,6 +3078,7 @@ const struct drm_ioctl_desc amdgpu_ioctls_kms[] = { > DRM_IOCTL_DEF_DRV(AMDGPU_USERQ_WAIT, amdgpu_userq_wait_ioctl, > DRM_AUTH|DRM_RENDER_ALLOW), > DRM_IOCTL_DEF_DRV(AMDGPU_GEM_LIST_HANDLES, > amdgpu_gem_list_handles_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), > DRM_IOCTL_DEF_DRV(AMDGPU_PROC_OPTIONS, amdgpu_proc_options_ioctl, > DRM_AUTH|DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF_DRV(AMDGPU_GEM_SVM, amdgpu_gem_svm_ioctl, > DRM_AUTH|DRM_RENDER_ALLOW), [Severity: High] Is there a Time-of-Check to Time-of-Use (TOCTOU) race condition when initializing SVM via this ioctl? When multiple threads concurrently issue the ioctl, they both might see vm->svm == NULL and proceed to allocate amdgpu_svm, initialize workqueues, and register MMU interval notifiers: if (!amdgpu_svm_is_enabled(vm)) { ret = amdgpu_svm_init(adev, vm); Could this lack of synchronization cause the first thread's allocation to be overwritten and permanently leaked, leaving dangling MMU notifiers registered on the mm struct? > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 2f3470208829e..9274b0359a164 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c [ ... ] > @@ -2763,6 +2769,9 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct > amdgpu_vm *vm) > unsigned long flags; > int i; > > + amdgpu_svm_close(vm); > + amdgpu_svm_fini(vm); > + > amdgpu_amdkfd_gpuvm_destroy_cb(adev, vm); > > root = amdgpu_bo_ref(vm->root.bo); [Severity: High] Can calling amdgpu_svm_fini() here cause a use-after-free if there is a concurrent page fault? amdgpu_svm_fini() destroys the internal SVM components before the PASID is removed from adev->vm_manager.pasids (which happens later in the function). If a concurrent page fault occurs, amdgpu_svm_handle_fault() might look up the SVM context by PASID, grab a reference, and dereference svm->attr_tree after it has been freed here. [ ... ] > @@ -2995,14 +3005,23 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device > *adev, u32 pasid, > return false; > > is_compute_context = vm->is_compute_context; > + use_amdgpu_svm = !!vm->svm; > > if (is_compute_context) { > /* Unreserve root since svm_range_restore_pages might try to > reserve it. */ > /* TODO: rework svm_range_restore_pages so that this isn't > necessary. */ > amdgpu_bo_unreserve(root); > > - if (!svm_range_restore_pages(adev, pasid, vmid, > - node_id, addr >> PAGE_SHIFT, ts, > write_fault)) { > + if (use_amdgpu_svm) > + r = amdgpu_svm_handle_fault(adev, pasid, > + addr >> PAGE_SHIFT, ts, > + write_fault); [Severity: High] Does amdgpu_svm_handle_fault() expect a byte address rather than a Page Frame Number (PFN)? Passing addr >> PAGE_SHIFT sends a PFN to amdgpu_svm_handle_fault(), which in turn passes it to amdgpu_gmc_filter_faults_remove(). Since the filter expects a byte address to compute its hash key (addr << 4 | pasid), the stale fault is never properly removed from the filter. This could cause legitimate new faults on the same page to be ignored, potentially hanging the GPU. > + else > + r = svm_range_restore_pages(adev, pasid, vmid, node_id, > + addr >> PAGE_SHIFT, ts, > + write_fault); [Severity: High] This is a pre-existing issue, but does svm_range_restore_pages() also suffer from the same PFN vs byte address bug? It receives addr >> PAGE_SHIFT here and passes it directly to amdgpu_gmc_filter_faults_remove(), which would fail to remove the fault from the GMC fault filter for the same reason. > + > + if (!r) { > amdgpu_bo_unref(&root); > return true; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
