Re: [PATCH v4 6/6] drm/amdgpu: introduce amdgpu_dev_enter / exit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential
issues to consider:
- [High] drm/amdgpu: UAF and leaks in amdgpu_drm_release() when device
  is wedged
- [High] drm/amdgpu: stale PTEs and broken open when wedged in GART/VM
  helpers
- [High] drm/amdgpu: uninitialized saved_bo copied to VRAM on reset of
  wedged GPU
- [High] drm/amdgpu: SR-IOV and Mode-2 resets fail to clear wedge_status
- [High] drm/amdgpu: NULL pointer dereference in amdgpu_vm_init() when
  wedged
- [High] drm/amdgpu: dangling entry->bo in amdgpu_vm_pt_alloc() when
  wedged
- [Medium] drm/amdgpu: WC memtype and MTRR leak in amdgpu_bo_fini() when
  wedged
- [Medium] drm/amdgpu: unplug deadlock in amdgpu_gmc_flush_gpu_tlb() on
  fence wait
- [Medium] drm/amdgpu: amdgpu_vram_mgr_dmem_reclaim() misses
  amdgpu_dev_enter()
--

commit 0d8a6bc2888d88350480e40fc5320cb5adf242ad
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

drm/amdgpu: introduce amdgpu_dev_enter / exit

This commit introduces amdgpu_dev_enter() and amdgpu_dev_exit() wrappers
that check whether the device is wedged before calling drm_dev_enter(), and
converts existing drm_dev_enter() and drm_dev_exit() call sites across
amdgpu.

Link: https://sashiko.dev/#/patchset/20261029152000.22001-1-pierre-eric.pelloux-prayer@amd.com?part=5

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 45680878aa5a1..4ba1772d9eb62 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
[ ... ]
> @@ -1384,4 +1385,17 @@ static inline bool amdgpu_device_is_wedged(struct amdgpu_device *adev)
>  	return atomic_read(&adev->wedge_status);
>  }
>  
> +static inline bool amdgpu_dev_enter(struct amdgpu_device *adev, int *idx)
> +{
> +	if (amdgpu_device_is_wedged(adev))
> +		return false;
> +
> +	return drm_dev_enter(adev_to_drm(adev), idx);
> +}
> +
> +static inline void amdgpu_dev_exit(struct amdgpu_device *adev, int idx)
> +{
> +	drm_dev_exit(idx);
> +}
> +
>  #endif

[Severity: Medium]
Should amdgpu_vram_mgr_dmem_reclaim() also use amdgpu_dev_enter() and
amdgpu_dev_exit()?

It still calls drm_dev_enter().  When dmem cgroup reclaim runs on a wedged
GPU, ttm_resource_manager_dmem_reclaim() -> amdgpu_bo_move() ->
amdgpu_move_blit() submits an SDMA copy job whose fence amdgpu_job_run()
immediately cancels and signals without copying, causing
ttm_bo_move_accel_cleanup() to free the source VRAM resource and lose the
BO contents.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 3e5634de077f1..4c3aa4e4d4c95 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -3030,13 +3030,13 @@ static int amdgpu_drm_release(struct inode *inode, struct file *filp)
>  	struct drm_device *dev = file_priv->minor->dev;
>  	int idx;
>  
> -	if (fpriv && drm_dev_enter(dev, &idx)) {
> +	if (fpriv && amdgpu_dev_enter(drm_to_adev(dev), &idx)) {
>  		amdgpu_evf_mgr_shutdown(&fpriv->evf_mgr);
>  		amdgpu_userq_mgr_cancel_resume(&fpriv->userq_mgr);
>  		amdgpu_evf_mgr_flush_suspend(&fpriv->evf_mgr);
>  		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>  		amdgpu_evf_mgr_fini(&fpriv->evf_mgr);
> -		drm_dev_exit(idx);
> +		amdgpu_dev_exit(drm_to_adev(dev), idx);
>  	}
>  
>  	return drm_release(inode, filp);

[Severity: High]
Does skipping userq_mgr and evf_mgr teardown when the device is wedged
cause a use-after-free on fpriv?

When a process closes its DRM fd while the GPU is wedged,
amdgpu_dev_enter() returns false and skips tearing down fpriv->userq_mgr
and fpriv->evf_mgr, after which drm_release() ->
amdgpu_driver_postclose_kms() unconditionally frees fpriv.  Any usermode
queues remain in adev->userq_doorbell_xa with queue->userq_mgr pointing
into the freed fpriv, which is dereferenced during GPU reset or suspend:

drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c:amdgpu_userq_mgr_cancel_reset_work() {
	...
	xa_for_each(xa, queue_id, queue) {
		cancel_delayed_work(&queue->hang_detect_work);
		cancel_work(&queue->userq_mgr->reset_work);
	}
}

Skipping teardown also leaves fpriv->evf_mgr.suspend_work,
fpriv->userq_mgr.resume_work and fpriv->userq_mgr.reset_work uncanceled,
and leaves unsignaled eviction fences on BOs with ev_fence->evf_mgr
pointing into the freed fpriv when amdgpu_eviction_fence_enable_signaling()
schedules suspend_work.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> index c4c21dbbbdbf8..62e98e50bf35a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> @@ -318,7 +318,7 @@ void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
>  	if (!adev->gart.ptr)
>  		return;
>  
> -	if (!drm_dev_enter(adev_to_drm(adev), &idx))
> +	if (!amdgpu_dev_enter(adev, &idx))
>  		return;
>  
>  	t = offset / AMDGPU_GPU_PAGE_SIZE;
> @@ -335,7 +335,7 @@ void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
>  	}
>  	amdgpu_gart_invalidate_tlb(adev);
>  
> -	drm_dev_exit(idx);
> +	amdgpu_dev_exit(adev, idx);
>  }

[Severity: High]
Does returning early from amdgpu_gart_unbind() when wedged leave stale GART
PTEs pointing to freed system pages across a subsequent GPU reset?

When a GTT BO is unbound while wedged, amdgpu_ttm_backend_unbind() marks
gtt->bound = false, TTM frees the backing system pages, and
amdgpu_gtt_mgr_del() frees the drm_mm_node without clearing the PTEs in
adev->gart.ptr to dummy_page_addr.  If the GPU later recovers via reset,
amdgpu_gtt_mgr_recover() only rewrites PTEs for still-allocated nodes in
mgr->mm, leaving the unallocated slots mapped to the freed pages when
adev->gart.ptr is in system RAM or preserved VRAM.

Similarly, returning -ENODEV when wedged from amdgpu_vm_update_range(),
amdgpu_vm_update_pdes() and amdgpu_vm_pt_clear() causes
amdgpu_vm_clear_freed() to drop mappings from vm->freed without clearing
their PTEs, logs dev_err in amdgpu_gem_object_close() on every closed BO,
and causes amdgpu_driver_open_kms() to fail with -ENODEV when opening the
DRM device node.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index b9afcbab02f9d..e435cb0c9d7f9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -728,9 +728,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>  	struct amdgpu_vmhub *hub = &adev->vmhub[vmhub];
>  	struct dma_fence *fence;
>  	struct amdgpu_job *job;
> -	int r;
> +	int r, idx;
>  
> -	if (amdgpu_device_is_wedged(adev))
> +	if (!amdgpu_dev_enter(adev, &idx))
>  		return;
[ ... ]
> @@ -782,11 +785,13 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>  
>  	dma_fence_wait(fence, false);
>  	dma_fence_put(fence);
> +	amdgpu_dev_exit(adev, idx);
>  
>  	return;

[Severity: Medium]
Can holding amdgpu_dev_enter() across dma_fence_wait() deadlock with
drm_dev_unplug() on hot-unplug?

CPU0                              CPU1
amdgpu_gmc_flush_gpu_tlb()
  amdgpu_dev_enter()
  fence = amdgpu_job_submit(job)
                                  amdgpu_pci_remove()
                                    drm_dev_unplug()
                                      dev->unplugged = true
                                      synchronize_srcu(&drm_unplug_srcu)
  dma_fence_wait(fence, false)
                                  amdgpu_job_timedout()
                                    drm_dev_enter() -> false
                                    return DRM_GPU_SCHED_STAT_ENODEV
                                      <-- fence is never signaled

When hub->sdma_invalidation_workaround is used, amdgpu_job_timedout() bails
out once dev->unplugged is set without force-completing the job fence, and
amdgpu_pci_remove() cannot reach amdgpu_fence_driver_hw_fini() until
synchronize_srcu(&drm_unplug_srcu) completes.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 6aa895f63aeaf..e0c1a876d76ad 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1144,12 +1144,12 @@ void amdgpu_bo_fini(struct amdgpu_device *adev)
>  
>  	amdgpu_ttm_fini(adev);
>  
> -	if (drm_dev_enter(adev_to_drm(adev), &idx)) {
> +	if (amdgpu_dev_enter(adev, &idx)) {
>  		if (!adev->gmc.xgmi.connected_to_cpu && !adev->gmc.is_app_apu) {
>  			arch_phys_wc_del(adev->gmc.vram_mtrr);
>  			arch_io_free_memtype_wc(adev->gmc.aper_base, adev->gmc.aper_size);
>  		}
> -		drm_dev_exit(idx);
> +		amdgpu_dev_exit(adev, idx);
>  	}
>  }

[Severity: Medium]
Does amdgpu_bo_fini() leak adev->gmc.vram_mtrr and the aperture WC memtype
reservation when tearing down a wedged device?

In amdgpu_device_fini_sw(), amdgpu_device_ip_fini() runs amdgpu_bo_fini()
before atomic_xchg(&adev->wedge_status, 0) clears wedge_status, so
amdgpu_dev_enter() returns false and skips arch_phys_wc_del() and
arch_io_free_memtype_wc() on a connected PCI device.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 82a2684dbd2e1..703f41b350aa9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
[ ... ]
> @@ -5155,19 +5155,19 @@ int psp_copy_fw(struct psp_context *psp, uint8_t *start_addr, uint32_t bin_size)
>  {
>  	int idx;
>  
> -	if (!drm_dev_enter(adev_to_drm(psp->adev), &idx))
> +	if (!amdgpu_dev_enter(psp->adev, &idx))
>  		return -ENODEV;
>  
>  	if (!bin_size || bin_size > PSP_1_MEG) {
>  		dev_err(psp->adev->dev, "PSP firmware is invalid\n");
> -		drm_dev_exit(idx);
> +		amdgpu_dev_exit(psp->adev, idx);
>  		return -EINVAL;
>  	}
>  
>  	memset(psp->fw_pri_buf, 0, PSP_1_MEG);
>  	memcpy(psp->fw_pri_buf, start_addr, bin_size);
>  
> -	drm_dev_exit(idx);
> +	amdgpu_dev_exit(psp->adev, idx);
>  	return 0;
>  }

[Severity: High]
Does converting psp_copy_fw() and amdgpu_gmc_init_pdb0() to
amdgpu_dev_enter() break SR-IOV and Mode-2 GPU reset recovery when the
device is wedged?

adev->wedge_status is only cleared during reset in
amdgpu_device_reinit_after_reset().  However, SR-IOV reset in
amdgpu_device_reset_sriov() and custom Mode-2 reset handlers in
amdgpu_do_asic_reset() never call amdgpu_device_reinit_after_reset().
During amdgpu_device_reset_sriov(), psp_copy_fw() fails with -ENODEV and
amdgpu_gmc_init_pdb0() silently skips PDB0 initialization because
wedge_status is still 1, and both reset paths leave the device wedged after
reset completes.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 6c12d8aa3c769..7477aed52eb1c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -352,9 +352,9 @@ static int amdgpu_vcn_save_vcpu_bo_inst(struct amdgpu_device *adev, int i)
>  	if (!adev->vcn.inst[i].saved_bo)
>  		return -ENOMEM;
>  
> -	if (drm_dev_enter(adev_to_drm(adev), &idx)) {
> +	if (amdgpu_dev_enter(adev, &idx)) {
>  		memcpy_fromio(adev->vcn.inst[i].saved_bo, ptr, size);
> -		drm_dev_exit(idx);
> +		amdgpu_dev_exit(adev, idx);
>  	}
>  
>  	return 0;
> @@ -403,9 +403,9 @@ int amdgpu_vcn_resume(struct amdgpu_device *adev, int i)
>  	ptr = adev->vcn.inst[i].cpu_addr;
>  
>  	if (adev->vcn.inst[i].saved_bo != NULL) {
> -		if (drm_dev_enter(adev_to_drm(adev), &idx)) {
> +		if (amdgpu_dev_enter(adev, &idx)) {
>  			memcpy_toio(ptr, adev->vcn.inst[i].saved_bo, size);
> -			drm_dev_exit(idx);
> +			amdgpu_dev_exit(adev, idx);
>  		}
>  		kvfree(adev->vcn.inst[i].saved_bo);
>  		adev->vcn.inst[i].saved_bo = NULL;

[Severity: High]
When resetting a wedged GPU, does this overwrite the VCPU buffer in VRAM
with uninitialized kernel memory?

During a reset of a wedged device, amdgpu_device_pre_asic_reset() runs
amdgpu_device_ip_suspend() while adev->wedge_status is still 1.
amdgpu_vcn_save_vcpu_bo_inst() allocates saved_bo with kvmalloc() and skips
memcpy_fromio() because amdgpu_dev_enter() returns false.  Then
amdgpu_device_reinit_after_reset() clears wedge_status to 0 and calls
amdgpu_vcn_resume(), where amdgpu_dev_enter() succeeds and memcpy_toio()
copies the uninitialized saved_bo into the VCPU BO.  The same pattern
occurs in amdgpu_uvd_prepare_suspend() / amdgpu_uvd_resume() and
vce_v4_0_suspend() / vce_v4_0_resume().

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index c73eb416c0e26..9d1fbddcdd7c9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -387,7 +387,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	if (r)
>  		return r;
>  
> -	if (!drm_dev_enter(adev_to_drm(adev), &idx))
> +	if (!amdgpu_dev_enter(adev, &idx))
>  		return -ENODEV;
>  
>  	r = vm->update_funcs->map_table(vmbo);

[Severity: High]
Does failing amdgpu_vm_pt_clear() when wedged trigger a NULL pointer
dereference in amdgpu_vm_init() whenever the DRM device is opened?

When a process opens the DRM device while wedged, amdgpu_driver_open_kms()
calls amdgpu_vm_init(), which calls amdgpu_vm_pt_clear() and jumps to
error_free_root on -ENODEV:

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:amdgpu_vm_init() {
	...
error_free_root:
	amdgpu_vm_pt_free_root(adev, vm);
	amdgpu_bo_unreserve(vm->root.bo);
	amdgpu_bo_unref(&root_bo);
}

amdgpu_vm_pt_free_root() calls amdgpu_vm_pt_free(&vm->root), which sets
vm->root.bo = NULL before amdgpu_bo_unreserve(vm->root.bo) dereferences it.

[Severity: High]
Can a -ENODEV return from amdgpu_vm_pt_clear() also leave a dangling
entry->bo pointer in amdgpu_vm_pt_alloc()?

In amdgpu_vm_pt_alloc(), vm->eviction_lock is dropped across
amdgpu_vm_pt_create(), during which a concurrent timeout can wedge the
device.  amdgpu_vm_pt_alloc() then initializes entry with pt_bo before
calling amdgpu_vm_pt_clear():

drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c:amdgpu_vm_pt_alloc() {
	...
	amdgpu_vm_bo_base_init(entry, p->vm, pt_bo);
	r = amdgpu_vm_pt_clear(p->adev, p->vm, pt, p->immediate);
	if (r)
		goto error_unpin;

	return 0;

error_unpin:
	if (p->vm->is_npa)
		amdgpu_bo_unpin(pt_bo);
	amdgpu_bo_unref(&pt_bo);
	return r;
}

When amdgpu_vm_pt_clear() returns -ENODEV, error_unpin frees pt_bo without
calling amdgpu_vm_pt_free(entry), leaving entry->bo pointing to freed
memory and entry->vm_status linked on the VM status list.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109171333.4329-1-pierre-eric.pelloux-prayer@amd.com?part=6