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 <[email protected]> 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/[email protected]?part=5
