Re: [PATCH] drm/amdgpu: replace kmalloc_array with kmalloc_objs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Synchronous dma_fence_wait() while holding dma_resv locks.
- [High] High-order kmalloc() driven by unprivileged user without __GFP_NOWARN fallback.
--

commit 80829efff044950d51193562a7bf95d3d15c515b
Author: Lalit Shankar Chowdhury <lalitshankarch@gmail.com>

drm/amdgpu: replace kmalloc_array with kmalloc_objs

Replace the remaining kmalloc_array instances with kmalloc_objs.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index 743b41db5b7cb..cbdbd8196eb28 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
[ ... ]
> @@ -726,13 +725,11 @@ amdgpu_userq_wait_return_fence_info(struct drm_device *dev, struct drm_file *fil
>  	struct drm_exec exec;
>  	int i, cnt, r;
>  
> -	fence_info = kmalloc_array(wait_info->num_fences, sizeof(*fence_info),
> -				   GFP_KERNEL);
> +	fence_info = kmalloc_objs(*fence_info, wait_info->num_fences);
>  	if (!fence_info)
>  		return -ENOMEM;
>  
> -	fences = kmalloc_array(wait_info->num_fences, sizeof(*fences),
> -			       GFP_KERNEL);
> +	fences = kmalloc_objs(*fences, wait_info->num_fences);

[Severity: High]
This isn't a bug introduced by this patch, but could these allocations trigger
a system-wide denial of service if wait_info->num_fences is large?

An unprivileged user can pass a large value for wait_info->num_fences via the
AMDGPU_USERQ_WAIT ioctl. Since this directly drives kmalloc_objs() using
GFP_KERNEL without __GFP_NOWARN, a high-order allocation failure on a
fragmented system could wake kswapd, stall compaction, and spam the kernel
log with massive stack traces.

Should these allocations be converted to use kvmalloc_array() or include
__GFP_NOWARN to prevent userspace abuse?

[Severity: High]
This is a pre-existing issue, but I noticed a potential deadlock later in
this function. Does this code safely wait on fences while holding dma_resv
locks?

Later in amdgpu_userq_wait_return_fence_info(), the driver locks the
user-provided GEM objects and iterates through their fences:

	drm_exec_until_all_locked(&exec) {
		r = drm_exec_prepare_array(&exec, gobj_read, ...);
		...
	}

	for (i = 0; i < num_read_bo_handles; i++) {
		dma_resv_for_each_fence(&resv_cursor, gobj_read[i]->resv,
					DMA_RESV_USAGE_READ, fence) {
			r = amdgpu_userq_wait_add_fence(wait_info, fences,
							&num_fences, fence);
			if (r)
				goto error_unlock;
		}
	}

If the number of iterated fences exceeds wait_info->num_fences,
amdgpu_userq_wait_add_fence() calls dma_fence_wait(fence, true) synchronously:

static int amdgpu_userq_wait_add_fence(...) {
	...
	if (*num_fences >= wait_info->num_fences)
		return dma_fence_wait(fence, true);
	...
}

Waiting on a dma_fence while holding a dma_resv lock can create a circular
lock dependency, because dma_fence signaling must not wait for dma_resv
locks, but reclaim under a dma_resv lock can wait on a dma_fence. Could this
reachable path from unprivileged userspace trigger a lockdep splat or
deadlock?

>  	if (!fences) {
>  		r = -ENOMEM;
>  		goto free_fence_info;

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