On 2026-11-06 09:22, Christian König wrote:
>
> On 11/6/26 15:18, Benjamin Cheng wrote:
>> 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 wanted to get that fixed for weeks but couldn't find time for it. Can you
>>> guys together look into that?
>>
>> I see, thanks I'll take a look at the previous patches.
>>
>> Should we consider reverting commit 009f401fb98b ("drm/amdgpu: move
>> amdgpu_vm_bo_update() call into correct location") in the meantime then? As
>> I wrote in the cover letter it exposes this issue and can now be detected by
>> IGT tests, which makes our CI flaky.
>>
> What is the rational that "drm/amdgpu: move amdgpu_vm_bo_update() call into
> correct location" should make this flaky?
>
> As far as I can see that is completely unrelated.
The amdgpu_hotunplug_with_cs test submits a CS while another thread unplugs the
device.
The commit made the seq64 mapping happen lazily on the first CS, and during the
PT handling for that, the unplug happens, causing amdgpu_vm_pt_clear() to fail,
triggering the use-after-free and leak of the SDMA job.
> Thanks,
> Christian.
>
>>> 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;
