Re: [PATCH] drm/amdgpu: fix NULL pointer deref in amdgpu_vm_handle_moved for BO-less mappings

On 8/5/26 11:01, Zhang, Jesse(Jie) wrote:
> AMD General
> 
>> -----Original Message-----
>> From: Koenig, Christian <[email protected]>
>> Sent: Wednesday, August 5, 2026 3:53 PM
>> To: Zhang, Jesse(Jie) <[email protected]>; [email protected]
>> Cc: Deucher, Alexander <[email protected]>
>> Subject: Re: [PATCH] drm/amdgpu: fix NULL pointer deref in
>> amdgpu_vm_handle_moved for BO-less mappings
>>
>> On 8/5/26 05:00, Zhang, Jesse(Jie) wrote:
>>> AMD General
>>>
>>>> -----Original Message-----
>>>> From: Koenig, Christian <[email protected]>
>>>> Sent: Tuesday, August 4, 2026 9:58 PM
>>>> To: Zhang, Jesse(Jie) <[email protected]>;
>>>> [email protected]
>>>> Cc: Deucher, Alexander <[email protected]>
>>>> Subject: Re: [PATCH] drm/amdgpu: fix NULL pointer deref in
>>>> amdgpu_vm_handle_moved for BO-less mappings
>>>>
>>>> On 7/27/26 10:40, Jesse Zhang wrote:
>>>>> The individual.needs_update loop in amdgpu_vm_handle_moved()
>>>>> unconditionally dereferenced bo_va->base.bo (to read its reservation
>>>>> object, ttm_tt and to test for a DMABuf import). However
>>>>> bo_va->base.bo can legitimately be NULL for PRT/sparse mappings -
>>>>> amdgpu_vm_bo_update() already handles a NULL bo via its "else if
>>>>> (!bo)" PRT path. When such a bo_va reaches the moved list (e.g. the
>>>>> userq eviction restore worker running amdgpu_vm_handle_moved() while
>>>>> a user
>>>> queue is being torn down during a GPU reset), the NULL deref crashes the
>> kernel:
>>>>>
>>>>>   BUG: kernel NULL pointer dereference, address: 0000000000000158
>>>>>   #PF: supervisor read access in kernel mode
>>>>>   Oops: 0000 [#1] SMP NOPTI
>>>>>   Workqueue: events amdgpu_userq_restore_worker [amdgpu]
>>>>>   RIP: 0010:amdgpu_vm_handle_moved+0x17a/0x200 [amdgpu]
>>>>>   Call Trace:
>>>>>    <TASK>
>>>>>    amdgpu_userq_vm_validate_and_restore_queue+0x2ce/0x920 [amdgpu]
>>>>>    amdgpu_userq_restore_worker+0xce/0x210 [amdgpu]
>>>>>    process_scheduled_works+0xa6/0x460
>>>>>    worker_thread+0x13c/0x290
>>>>>    kthread+0xfb/0x140
>>>>>    ret_from_fork+0x1b6/0x2b0
>>>>>    ret_from_fork_asm+0x1a/0x30
>>>>>    </TASK>
>>>>>
>>>>> The faulting instruction is "mov rax,[rdx+0x158]" with rdx (bo) == 0
>>>>> and
>>>>> CR2 == 0x158, i.e. reading bo->tbo.base.resv off a NULL bo.
>>>>>
>>>>> Guard the BO-less case: skip the reservation dance and the
>>>>> DMABuf-import check when bo is NULL, and let amdgpu_vm_bo_update()
>>>>> take
>>>> its existing PRT path.
>>>>
>>>> Clear NAK. PRT mappings should *never* be on the moved list in the first place.
>>> Thanks for the review.
>>> I tracked the issue to the state-machine path: amdgpu_vm_bo_update()
>>> can put a NULL-bo bo_va onto individual.idle via amdgpu_vm_bo_idle(),
>>> and then
>>> amdgpu_vm_bo_reset_state_machine() promotes it to individual.needs_update
>> after a VM generation reset.
>>
>> Something is seriously wrong here. I've fixed
>> amdgpu_vm_bo_reset_state_machine() to not do that weeks ago.
>>
>> Where do you see that issue? On some release branch?
> Hi Christian,
> 
> Thanks — I found the fix you mean: 989bdec8ca04 ("drm/amdgpu: fix amdgpu_vm_bo_reset_state_machine", 2026-05-22)。 It's already in amd-staging-drm-next.
> 
> But that commit addresses a different problem: it replaces the list_splice in reset_state_machine() so the root PD gets its special handling and the moved flag is set correctly.
> It does not touch the NULL-bo case — after it, an entry on individual.idle is still list_move'd onto individual.needs_update.
> 
> The crash I'm reporting has a different root cause: a PRT/sparse mapping's bo_va has base.bo == NULL (fpriv->prt_va, created via amdgpu_vm_bo_add(vm, NULL)).
> The tail of amdgpu_vm_bo_update() unconditionally calls amdgpu_vm_bo_idle() in the !always_valid else branch, putting that NULL-bo bo_va onto individual.idle.
> After a GPU reset reset_state_machine() promotes it to individual.needs_update, and amdgpu_vm_handle_moved() then dereferences bo_va->base.bo unconditionally.
> 
> I can still reproduce this on the latest amd-staging-drm-next (HEAD includes 989bdec8ca04).
> the log:
> [ 3107.632098] RIP: 0010:amdgpu_vm_handle_moved+0x17a/0x200 [amdgpu]
> [ 3107.632469] Code: e8 5b c7 f9 ee 49 8b 84 24 60 02 00 00 48 3b 45 b8 0f 84 45 ff ff ff 4d 8b ac 24 60 02 00 00 4c 89 ff 49 8b 55 f0 4d 8d 75 e8 <48> 8b 82 58 01 00 00 48 89 55 c8 48 89 45 d0 e8 92 c0 f9 ee 0f b6
> [ 3107.632478] RSP: 0018:ffffcda785f23cb0 EFLAGS: 00010283
> [ 3107.632484] RAX: ffff8ad9cc15fe98 RBX: ffff8ada86100000 RCX: ffff8ad9cf4a8188
> [ 3107.632488] RDX: 0000000000000000 RSI: 000000000000000f RDI: ffff8adab01c0248
> [ 3107.632493] RBP: ffffcda785f23d00 R08: 0000000000000000 R09: 0000000000000004
> [ 3107.632497] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8adab01c0000
> [ 3107.632501] R13: ffff8ad9cc15fe98 R14: ffff8ad9cc15fe80 R15: ffff8adab01c0248
> [ 3107.632505] FS:  0000000000000000(0000) GS:ffff8add3c8ff000(0000) knlGS:0000000000000000
> [ 3107.632510] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3107.632515] CR2: 0000000000000158 CR3: 0000000117903000 CR4: 0000000000750ef0
> [ 3107.632520] PKRU: 55555554
> [ 3107.632523] Call Trace:
> [ 3107.632527]  <TASK>
> [ 3107.632533]  amdgpu_userq_vm_validate_and_restore_queue+0x2ce/0x920 [amdgpu]
> [ 3107.632846]  amdgpu_userq_restore_worker+0xa6/0x180 [amdgpu]
> [ 3107.633036]  process_scheduled_works+0xa6/0x460
> [ 3107.633042]  worker_thread+0x13c/0x290
> [ 3107.633045]  kthread+0xfb/0x140
> [ 3107.633048]  ? __pfx_worker_thread+0x10/0x10
> [ 3107.633050]  ? __pfx_kthread+0x10/0x10
> [ 3107.633053]  ret_from_fork+0x1b6/0x2b0
> [ 3107.633056]  ? __pfx_kthread+0x10/0x10
> [ 3107.633058]  ret_from_fork_asm+0x1a/0x30

There is still something going wrong here.

The PRT should be updated in the PTEs separately, otherwise we use the wrong fence to sync it.

There should be something like this in the userq validation code as well:

        r = amdgpu_vm_bo_update(adev, fpriv->prt_va, false);
        if (r) 
                return r;
        
        r = amdgpu_sync_fence(&p->sync, fpriv->prt_va->last_pt_update,
                              GFP_KERNEL);
...

Similar to how we have it in the CS code.

Regards,
Christian.

> 
> 
> So this is a distinct issue from 989bdec8ca04.
> My v2 keeps the PRT bo_va off the state lists at the producer side (amdgpu_vm_bo_update: else → else if (bo)), which is what your earlier feedback asked for.
> 
> Thanks,
> Jesse
>>
>> Regards,
>> Christian.
>>
>>>
>>> I’ll send v2 that fixes this at the producer side by keeping NULL-bo
>>> (PRT) mappings off the vm_bo state lists, and I’ll drop the
>>> consumer-side NULL guard from v1.
>>>
>>> Thanks,
>>> Jesse
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Signed-off-by: Jesse Zhang <[email protected]>
>>>>> ---
>>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 12 +++++++++---
>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index aac8ace9d7a6..fbb76c32bff9 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -1631,11 +1631,17 @@ int amdgpu_vm_handle_moved(struct
>>>> amdgpu_device *adev,
>>>>>             bo_va = list_first_entry(&vm->individual.needs_update,
>>>>>                                      typeof(*bo_va), base.vm_status);
>>>>>             bo = bo_va->base.bo;
>>>>> -           resv = bo->tbo.base.resv;
>>>>> +           resv = bo ? bo->tbo.base.resv : NULL;
>>>>>             spin_unlock(&vm->individual_lock);
>>>>>
>>>>> +           /* PRT/sparse mappings have no BO to reserve; just update the
>>>>> +            * page tables (amdgpu_vm_bo_update() handles a NULL bo).
>>>>> +            */
>>>>> +           if (!bo) {
>>>>> +                   clear = false;
>>>>> +                   unlock = false;
>>>>>             /* Try to reserve the BO to avoid clearing its ptes */
>>>>> -           if (!adev->debug_vm && !amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)
>>>> &&
>>>>> +           } else if (!adev->debug_vm &&
>>>>> +!amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) &&
>>>>>                 dma_resv_trylock(resv)) {
>>>>>                     clear = false;
>>>>>                     unlock = true;
>>>>> @@ -1659,7 +1665,7 @@ int amdgpu_vm_handle_moved(struct
>>>>> amdgpu_device
>>>> *adev,
>>>>>             /* Remember evicted DMABuf imports in compute VMs for later
>>>>>              * validation
>>>>>              */
>>>>> -           if (vm->is_compute_context &&
>>>>> +           if (bo && vm->is_compute_context &&
>>>>>                 drm_gem_is_imported(&bo_va->base.bo->tbo.base) &&
>>>>>                 (!bo_va->base.bo->tbo.resource ||
>>>>>                  bo_va->base.bo->tbo.resource->mem_type ==
>>>> TTM_PL_SYSTEM))
>>>
>