Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] amdgpu_vm_clear_freed errors are ignored in KFD BO unmap and GEM close paths, leaving active PTEs mapped to freed memory. A running user-mode queue shader can continue to access the memory after it has been reallocated. -- commit 16b841a63bb4a2522f39993f74bd498158fed54e Author: Thadeu Lima de Souza Cascardo <[email protected]> drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails This commit modifies amdgpu_vm_clear_freed to avoid freeing a mapping and removing it from the freed list if amdgpu_vm_update_range fails. It assumes that leaving the mapping in the freed list will allow a subsequent amdgpu_cs_ioctl to retry clearing it, preventing invalid access. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 09037df76400d..5bde36754607f 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev, > while (!list_empty(&vm->freed)) { > mapping = list_first_entry(&vm->freed, > struct amdgpu_bo_va_mapping, list); > - list_del(&mapping->list); > > r = amdgpu_vm_update_range(adev, vm, false, false, true, false, > &sync, mapping->start, mapping->last, > 0, 0, 0, NULL, NULL, &f); > - amdgpu_vm_free_mapping(adev, vm, mapping, f); > if (r) { > dma_fence_put(f); > goto error_free; > } [Severity: Critical] This is a pre-existing issue, but does leaving the mapping in the freed list actually guarantee it will be cleared later? The commit message assumes that a later amdgpu_cs_ioctl will retry the clearing. However, are there paths that bypass the CS ioctl where this error is ignored? For instance, in KFD unmap_bo_from_gpuvm in drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c, the error from amdgpu_vm_clear_freed is explicitly ignored: (void)amdgpu_vm_clear_freed(adev, vm, &bo_va->last_pt_update); This marks the BO as unmapped in software, allowing it to be subsequently freed via amdgpu_amdkfd_gpuvm_free_memory_of_gpu without waiting for the PTEs to be cleared. Because KFD uses hardware user-mode queues, the clearing is never retried by a CS ioctl. A running user-mode queue shader could continue to access the freed memory. Similarly, in amdgpu_gem_object_close in drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c, the error is ignored and it bails out without attaching a fence: r = amdgpu_vm_clear_freed(adev, vm, &fence); if (unlikely(r < 0) && !drm_dev_is_unplugged(adev_to_drm(adev))) dev_err(adev->dev, "failed to clear page " "tables on GEM object close (%ld)\n", r); if (r || !fence) goto out_unlock; Could this leave active PTEs mapped to freed memory, leading to a potential information leak or memory corruption if amdgpu_vm_update_range fails (e.g., due to an early -ENOMEM)? > + list_del(&mapping->list); > + amdgpu_vm_free_mapping(adev, vm, mapping, f); > } > > if (fence && f) { -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
