Re: [PATCH 4/4] drm/amdgpu: commit pending work when amdgpu_vm_ptes_update fails

[sashiko-bot@kernel.org]

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 <cascardo@igalia.com>

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(&params, 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/20260914-amdgpu_vm_fixes-v1-0-d6d835591e0f@igalia.com?part=4