Re: [PATCH 5/5] drm/amdgpu: free userptr HMM ranges on the CS error path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Data Race / Use-After-Free due to shared amdgpu_bo_list_entry
- [High] DRM scheduler API contract violation: aborting job after drm_sched_job_arm()
--

--- Patch [5]: [PATCH 5/5] drm/amdgpu: free userptr HMM ranges on the CS error path ---
commit c42f00dbd4932bbfbbb760f3bcc81040f9afbef4
Author: Junrui Luo <moonafterrain@outlook.com>

drm/amdgpu: free userptr HMM ranges on the CS error path

This commit fixes a memory leak in the amdgpu_cs ioctl error path by
releasing userptr HMM ranges in amdgpu_cs_parser_fini(). Previously, these
ranges were only freed on success or specific error paths, leading to leaks
when errors occurred between allocation and the release loop in
amdgpu_cs_submit().

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 617f53f135f35..17c4fec214028 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1416,6 +1416,16 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
>  				amdgpu_vm_bo_invalidate(bo, false);
>  			}
>  		}
> +
> +		/*
> +		 * Release the ranges still live on the error paths;
> +		 * amdgpu_cs_submit() already freed and cleared them when it
> +		 * got far enough to check them for invalidation.
> +		 */
> +		amdgpu_bo_list_for_each_userptr_entry(e, parser->bo_list) {
> +			amdgpu_hmm_range_free(e->range);
> +			e->range = NULL;

[Severity: Critical]
This is a pre-existing issue, but does lockless modification of the shared
bo_list_entry introduce a data race or use-after-free here?

If userspace creates a BO list, obtains a handle, and invokes multiple
concurrent amdgpu_cs ioctls using the same BO list handle, the amdgpu_bo_list
objects are reference-counted and shared across those ioctls.

If one thread fails during the ioctl and reaches this newly modified
amdgpu_cs_parser_fini(), could it free e->range while another thread is
concurrently allocating or using it in amdgpu_cs_parser_bos()?

Thread 1:
amdgpu_cs_parser_bos() {
    ...
    e->range = amdgpu_hmm_range_alloc(NULL);
    ...
}

Thread 2 (on an error path):
amdgpu_cs_parser_fini() {
    ...
    amdgpu_bo_list_for_each_userptr_entry(e, parser->bo_list) {
        amdgpu_hmm_range_free(e->range);
        e->range = NULL;
    }
}

> +		}
>  		amdgpu_bo_list_put(parser->bo_list);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does amdgpu_cs_submit() violate
the DRM scheduler API contract by aborting jobs after they are armed?

Looking at amdgpu_cs_submit(), it arms jobs but can return an error before
pushing them:

amdgpu_cs_submit() {
    ...
    for (i = 0; i < p->gang_size; ++i)
        drm_sched_job_arm(&p->jobs[i]->base);
    ...
    if (r || !list_empty(&vm->individual.needs_update)) {
        r = -EAGAIN;
        mutex_unlock(&p->adev->notifier_lock);
        return r;
    }
    ...
}

If the function returns an error (e.g., if drm_sched_job_add_dependency() fails
or !list_empty(&vm->individual.needs_update) is true), the armed job is
aborted via drm_sched_job_cleanup() instead of being submitted.

Does calling drm_sched_job_cleanup() after drm_sched_job_arm() leave sequence
gaps on the scheduler entity timeline, potentially causing permanent GPU hangs
for subsequent jobs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-amdgpu-fixes-v1-0-4954a417b8ff@outlook.com?part=5