Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [High] Memory leak of the dynamically allocated `scheds` array on the success path when `rdev->num_cores == 1`. - [High] Use-after-free due to `sched_list` being freed before destroying the DRM scheduler entity. - [High] API contract violation and fence/job leak on the error path after arming the scheduler job. - [High] Silent discarding of job submission errors in the ioctl handler. - [High] Data race on input buffer objects due to missing read dependency fences. -- --- Patch [1]: [PATCH v2] drm/rocket: Check allocations before use --- commit c8fde6c32c7f279bae49a07582e5353988f62959 Author: Triet Hoang <[email protected]> drm/rocket: Check allocations before use Check the result of kvmalloc_array() in rocket_job_push() and kmalloc_objs() in rocket_job_open() before using the allocated buffer. > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index ac51bff39833f..24e1a61f71e7b 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -192,6 +192,9 @@ static int rocket_job_push(struct rocket_job *job) > > bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void > *), > GFP_KERNEL); > + if (!bos) > + return -ENOMEM; > + > memcpy(bos, job->in_bos, job->in_bo_count * sizeof(void *)); > memcpy(&bos[job->in_bo_count], job->out_bos, job->out_bo_count * > sizeof(void *)); [Severity: High] This is a pre-existing issue, but does the error path later in this function leak the job and fence? After drm_sched_job_arm() is called, the job's s_fence is allocated and an extra reference is taken via dma_fence_get(). If rocket_acquire_object_fences() subsequently fails, the code jumps to err_unlock: rocket_job_push() { ... drm_sched_job_arm(&job->base); job->inference_done_fence = dma_fence_get(&job->base.s_fence->finished); ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, &job->base, false); if (ret) goto err_unlock; ... } Since this path does not call drm_sched_job_cleanup() or release the fence reference via dma_fence_put(), does it leak these resources? Furthermore, since drm_sched_job_arm() is a point of no return according to the DRM scheduler rules, shouldn't all fallible operations be safely completed before arming the job? [Severity: High] This is also a pre-existing issue in rocket_job_push(), but does this omit read dependencies for the input buffer objects? At the end of the function, the execution fence is successfully attached to the output buffers: rocket_job_push() { ... rocket_attach_object_fences(job->out_bos, job->out_bo_count, job->inference_done_fence); ... } However, it seems to skip doing the same for job->in_bos with a DMA_RESV_USAGE_READ dependency. If userspace submits a read job and then a write job to the same buffer object, will the write job fail to wait for the read to finish, potentially overwriting data while the NPU is still executing? [ ... ] > @@ -499,7 +502,10 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv) > struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds, > rdev->num_cores); > unsigned int core; > - int ret; > + int ret = 0; > + > + if (!scheds) > + return -ENOMEM; > > for (core = 0; core < rdev->num_cores; core++) > scheds[core] = &rdev->cores[core].sched; > @@ -509,9 +515,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv) > scheds, > rdev->num_cores, NULL); > if (WARN_ON(ret)) > - return ret; > + kfree(scheds); > > - return 0; > + return ret; > } [Severity: High] This isn't a bug introduced by this patch, but is it possible that the scheds array is leaked on the success path when rdev->num_cores is 1? Looking at drm_sched_entity_init(), it sets entity->sched_list to NULL if num_sched_list is 1. If rocket_job_open() returns success here, the scheds pointer is not tracked anywhere else. Later, when an unprivileged userspace application closes the device file and rocket_job_close() is called, kfree(entity->sched_list) will evaluate to kfree(NULL), leading to a memory leak per file open. > > void rocket_job_close(struct rocket_file_priv *rocket_priv) [Severity: High] This is a pre-existing issue, but does this sequence cause a use-after-free in rocket_job_close()? rocket_job_close() { ... kfree(entity->sched_list); drm_sched_entity_destroy(entity); } By calling kfree(entity->sched_list) immediately before drm_sched_entity_destroy(entity), the memory is freed while the entity might still be active and flushing pending jobs. If background scheduler threads attempt to evaluate runqueues or access sched_list during the teardown phase in drm_sched_entity_destroy(), will they access prematurely freed memory? While reviewing the file, I noticed another pre-existing issue outside of the modified hunks. [Severity: High] This is a pre-existing issue, but does this loop silently discard job submission errors? rocket_ioctl_submit() { ... for (i = 0; i < args->job_count; i++) rocket_ioctl_submit_job(dev, file, &jobs[i]); ... } By ignoring the return value of rocket_ioctl_submit_job(), if submitting a job fails due to memory allocation failure, invalid task count, or failed GEM lookups, the loop continues and eventually returns 0 to userspace. Will this give userspace no indication that one or more jobs were silently dropped, potentially causing applications to hang while waiting for jobs that were never pushed? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
