Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Ignored return value of update_funcs->update leads to NULL pointer dereference or silent GPU page faults. - [High] Dangling pointer / Use-After-Free in amdgpu_vm_pt_alloc when amdgpu_vm_pt_clear fails. -- commit 7a14ea1f8dd61c2e4083bcbdd9192e590552290f Author: Thadeu Lima de Souza Cascardo <[email protected]> drm/amdgpu: commit pending work when amdgpu_vm_ptes_update fails This commit changes the error path when amdgpu_vm_ptes_update() fails to break the loop and commit pending work instead of jumping to error_free. This fixes an SDMA job memory leak and ensures TLB entries are flushed and local stack waitlists are cleared properly. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 5bde36754607f..1495adaaacd89 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -1235,7 +1235,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, > struct amdgpu_vm *vm, > tmp = start + num_entries; > r = amdgpu_vm_ptes_update(¶ms, start, tmp, addr, flags); > if (r) > - goto error_free; > + break; [Severity: High] This is a pre-existing issue, but I noticed a potential use-after-free in the page table allocation path called from amdgpu_vm_ptes_update(): drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c:amdgpu_vm_pt_alloc() { ... error_free_pt: amdgpu_bo_unref(&pt_bo); return r; } If amdgpu_vm_pt_clear() fails (e.g., due to memory allocation failure or fence wait timeout), it jumps to error_free_pt and frees the newly allocated pt_bo. However, this BO has already been assigned to entry->bo and added to VM tracking lists by the preceding call to amdgpu_vm_bo_base_init(). Is it possible that freeing pt_bo here leaves entry->bo pointing to the freed memory? [Severity: Critical] This is a pre-existing issue, but there appears to be an unhandled error path during PTE updates called from amdgpu_vm_ptes_update(): drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c:amdgpu_vm_pte_update_flags() { ... params->vm->update_funcs->update(params, pt, pe, addr, count, incr, flags); } The update function's return value is completely ignored. If the update fails (for example, if amdgpu_vm_sdma_update() fails to allocate a job due to ENOMEM), the job pointer (p->job) is set to NULL. Since the error is dropped, amdgpu_vm_ptes_update() continues iterating and calls amdgpu_vm_pte_update_flags() again. Will this cause amdgpu_vm_sdma_update() to dereference the NULL p->job on the next iteration, leading to a kernel panic? Or if it's a CPU update, will ignoring the failure lead to invalid GPU page tables and subsequent GPU page faults? > > amdgpu_res_next(&cursor, num_entries * AMDGPU_GPU_PAGE_SIZE); > start = tmp; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
