Re: [PATCH v3] drm/amdgpu: reject mapping info when BO VA is gone

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch adds a duplicate check for a missing BO VA, which changes the API return code and renders the existing check as unreachable dead code.

Pre-existing issues:
- [High] Potential deadlock due to allocating memory with `GFP_KERNEL` while holding `dma_resv` locks.
--

commit 6d1719ca92ca060fce9b1b2b5a70616a3a67fa1c
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

drm/amdgpu: reject mapping info when BO VA is gone

This commit adds a check in amdgpu_gem_op_ioctl() to reject the
AMDGPU_GEM_OP_GET_MAPPING_INFO operation if the BO VA is missing.
The intention is to prevent a NULL pointer dereference when a concurrent
handle close removes the per-file BO VA before the mapping list is walked.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -1087,6 +1087,12 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
[ ... ]
>  		int num_mappings = 0;
> +
> +		if (!bo_va) {
> +			r = -EINVAL;
> +			goto out_exec;
> +		}
> +

[Severity: Critical]
Does adding this check break userspace ABI by changing the error code?

There is a pre-existing check for a NULL bo_va slightly lower in this
function that correctly returns -ENOENT. By returning -EINVAL here,
applications expecting the standard -ENOENT for a missing BO VA might
fail unexpectedly.

[ ... ]
>  		/*
>  		 * num_entries is set as an input to the size of the user-allocated array of
[ ... ]
>  		 * be retried.
>  		 */
>  		if (!bo_va) {
>  			r = -ENOENT;
>  			goto out_exec;
>  		}

The newly added check above also makes this original check unreachable dead
code. 

[ ... ]
>  		if (args->num_entries > INT_MAX / sizeof(*vm_entries)) {
>  			r = -EINVAL;
>  			goto out_exec;
>  		}
>  
>  		vm_entries = kvcalloc(args->num_entries, sizeof(*vm_entries), GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but can allocating memory with GFP_KERNEL here
cause a deadlock?

This allocation occurs while sleepable dma_resv locks are held (acquired via
drm_exec_lock_obj() and amdgpu_vm_lock_pd() earlier in the function).

Using GFP_KERNEL allows direct memory reclaim. If system memory pressure
forces the kernel to invoke GPU shrinkers while this thread holds dma_resv
locks, the shrinker may attempt to lock the same BO or VM root PD, leading
to a deadlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629152807.13492-1-alhouseenyousef@gmail.com?part=1