Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] amdgpu_vm_clear_freed errors are ignored in KFD BO unmap and GEM close paths, leaving active PTEs mapped to freed memory. A running user-mode queue shader can continue to access the memory after it has been reallocated.
--

commit 16b841a63bb4a2522f39993f74bd498158fed54e
Author: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

This commit modifies amdgpu_vm_clear_freed to avoid freeing a mapping and
removing it from the freed list if amdgpu_vm_update_range fails. It assumes
that leaving the mapping in the freed list will allow a subsequent
amdgpu_cs_ioctl to retry clearing it, preventing invalid access.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 09037df76400d..5bde36754607f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>  	while (!list_empty(&vm->freed)) {
>  		mapping = list_first_entry(&vm->freed,
>  			struct amdgpu_bo_va_mapping, list);
> -		list_del(&mapping->list);
>  
>  		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>  					   &sync, mapping->start, mapping->last,
>  					   0, 0, 0, NULL, NULL, &f);
> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>  		if (r) {
>  			dma_fence_put(f);
>  			goto error_free;
>  		}

[Severity: Critical]
This is a pre-existing issue, but does leaving the mapping in the freed list
actually guarantee it will be cleared later?

The commit message assumes that a later amdgpu_cs_ioctl will retry the
clearing. However, are there paths that bypass the CS ioctl where this error
is ignored?

For instance, in KFD unmap_bo_from_gpuvm in
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c, the error from
amdgpu_vm_clear_freed is explicitly ignored:

	(void)amdgpu_vm_clear_freed(adev, vm, &bo_va->last_pt_update);

This marks the BO as unmapped in software, allowing it to be subsequently
freed via amdgpu_amdkfd_gpuvm_free_memory_of_gpu without waiting for the
PTEs to be cleared. Because KFD uses hardware user-mode queues, the clearing
is never retried by a CS ioctl. A running user-mode queue shader could
continue to access the freed memory.

Similarly, in amdgpu_gem_object_close in
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c, the error is ignored and it bails
out without attaching a fence:

	r = amdgpu_vm_clear_freed(adev, vm, &fence);
	if (unlikely(r < 0) && !drm_dev_is_unplugged(adev_to_drm(adev)))
		dev_err(adev->dev, "failed to clear page "
			"tables on GEM object close (%ld)\n", r);
	if (r || !fence)
		goto out_unlock;

Could this leave active PTEs mapped to freed memory, leading to a potential
information leak or memory corruption if amdgpu_vm_update_range fails
(e.g., due to an early -ENOMEM)?

> +		list_del(&mapping->list);
> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>  	}
>  
>  	if (fence && f) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-amdgpu_vm_fixes-v1-0-d6d835591e0f@igalia.com?part=3