Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Kernel memory allocation for SVM ranges and attribute ranges omits memcg accounting, allowing userspace to bypass memory cgroups. - [High] Lockless publication and clearing of `vm->svm` causes a data race, leading to uninitialized memory reads and Use-After-Free during PASID lookup. - [High] Premature destruction of `amdgpu_svm` internal resources before the reference count reaches zero leads to Use-After-Free. - [Medium] Attribute changes to the 0th page (virtual address 0) are silently ignored due to a flawed empty-range check. - [Medium] Slab caches allocated during initialization are never destroyed, causing a memory leak on module unload. --
commit ccb807100535d706e39546251263d3b6b1ad0ff1 Author: Honglei Huang <[email protected]> drm/amdgpu: implement SVM initialization and lifecycle This commit implements the amdgpu_svm.c core module for AMDGPU SVM, handling initialization, TLB flushing, slab cache management, attribute change detection, and kref-based lifecycle management. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm.c --- /dev/null +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm.c [ ... ] > +static struct drm_gpusvm_range *amdgpu_svm_range_alloc(struct drm_gpusvm > *gpusvm) > +{ > + struct amdgpu_svm_range *range; > + > + range = kmem_cache_zalloc(amdgpu_svm_range_cache, GFP_KERNEL); [Severity: High] Does this allocation need memcg accounting? Since userspace controls the number of ranges allocated, omitting the accounting flags could allow a malicious process to exhaust kernel memory and bypass memory cgroup limits. Should amdgpu_svm_range_cache be created with the account flag, or should GFP_KERNEL include the account bit here? > + if (!range) > + return NULL; [ ... ] > +struct amdgpu_svm * > +amdgpu_svm_lookup_by_pasid(struct amdgpu_device *adev, uint32_t pasid) > +{ > + struct amdgpu_svm *svm = NULL; > + struct amdgpu_vm *vm; > + unsigned long irqflags; > + > + xa_lock_irqsave(&adev->vm_manager.pasids, irqflags); > + vm = xa_load(&adev->vm_manager.pasids, pasid); > + if (vm && vm->svm) { > + svm = vm->svm; > + kref_get(&svm->refcount); > + } [Severity: High] Is it safe to read vm->svm and increment its refcount locklessly here? If amdgpu_svm_fini() concurrently clears vm->svm to NULL without synchronization, a page fault thread might load a pointer to an object that is actively being freed. Could this lead to a use-after-free crash if the clear and lookup are not synchronized? > + xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags); > + > + return svm; > +} [ ... ] > +void amdgpu_svm_cache_fini(void) > +{ > + if (!amdgpu_svm_range_cache) > + return; > + > + amdgpu_svm_attr_cache_fini(); > + AMDGPU_SVM_KMEM_CACHE_DESTROY(amdgpu_svm_range_cache); > +} [Severity: Medium] Is this cleanup function wired up to the module exit paths? It appears to be defined but never called anywhere in the teardown sequences. Will this leak the slab caches when the module is unloaded? [ ... ] > +int amdgpu_svm_apply_attr_change(struct amdgpu_svm *svm, > + const struct amdgpu_svm_attrs *old_attrs, > + const struct amdgpu_svm_attrs *new_attrs, > + unsigned long start_page, > + unsigned long last_page) > +{ > + bool old_access, new_access; > + bool needs_invalidate = false; > + bool needs_mapping = false; > + uint32_t trigger; > + int ret; > + > + amdgpu_svm_assert_locked(svm); > + > + if (!start_page && !last_page) > + return 0; [Severity: Medium] Does this condition unintentionally skip valid single-page mappings at virtual address zero? If a userspace application legitimately modifies attributes for the 0th page, start_page and last_page would both be 0, causing this function to silently abort without updating hardware mappings or invalidating the TLB. > + > + trigger = attr_change_trigger(old_attrs, new_attrs); [ ... ] > +void amdgpu_svm_fini(struct amdgpu_vm *vm) > +{ > + struct amdgpu_svm *svm = vm->svm; > + > + if (!svm) > + return; > + > + amdgpu_svm_close(vm); > + amdgpu_svm_lock(svm); > + drm_gpusvm_fini(&svm->gpusvm); > + amdgpu_svm_unlock(svm); > + > + amdgpu_svm_attr_tree_destroy(svm->attr_tree); > + amdgpu_svm_work_fini(svm); > + vm->svm = NULL; > + amdgpu_svm_put(svm); > +} [Severity: High] Should the destruction of embedded resources happen in amdgpu_svm_release instead of here? The svm object is reference counted, meaning concurrent threads could hold an active reference while this runs. Tearing down the resources here before dropping the final reference with amdgpu_svm_put could result in a use-after-free crash if a concurrent thread accesses these subsystems. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
