On 2026-11-06 03:55, Christian König wrote:
> On 11/5/26 22:00, Benjamin Cheng wrote:
>> In the happy path, the job gets freed when the job is submitted.
>> However, if amdgpu_vm_ptes_update failed, we don't submit the job, so we
>> would leak the job.
> Yeah that was already brought up upstream as well, but that here is the wrong
> approach to fixing this.
>
> Thadeu Lima de Souza Cascardo <[email protected]> came up with a few
> patches on the public mailing list to tackle this as well.
>
> Essentially we need to commit PTE updates before we allocate any new PT,
> background is not only that allocating PTs can fail but also that we need to
> drop the lock for this and that can make the PTEs we already constructed in
> the SDMA IB invalid.
I gave this some more thought. I think these two fixes are still needed.
If we commit the PTE updates before allocating PTs, the pt_alloc could still
fail and we would run into the same leak/use-after-free that we do now.
I think we need to both commit the PTE updates before allocating and have these
two fixes?
Best,
Ben
> I wanted to get that fixed for weeks but couldn't find time for it. Can you
> guys together look into that?
>
> Thanks in advance,
> Christian.
>
>> Fixes: 009f401fb98b ("drm/amdgpu: move amdgpu_vm_bo_update() call into
>> correct location")
>> Signed-off-by: Benjamin Cheng <[email protected]>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 2ba33212d507..f6cb2b5854dd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1274,8 +1274,11 @@ 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)
>> + if (r) {
>> + if (params.job)
>> + amdgpu_job_free(params.job);
>> goto error_free;
>> + }
>>
>> amdgpu_res_next(&cursor, num_entries * AMDGPU_GPU_PAGE_SIZE);
>> start = tmp;
