Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Critical NULL pointer dereference in `rocket_job_cleanup()` leading to kernel panic. - [High] Memory leak of `scheds` array in `rocket_job_open` on success. - [High] Fallible operations placed after `drm_sched_job_arm()` violate the strict DRM scheduler contract. - [High] Ignored return value in `rocket_ioctl_submit()` silently swallows failed job submissions. -- --- Patch [1]: [PATCH v3] drm/rocket: Check allocations before use --- commit e005f1f8daeacc5c010970453e21490c444073da Author: Triet Hoang <[email protected]> drm/rocket: Check allocations before use This commit adds missing allocation failure checks for kvmalloc_array() in rocket_job_push() and kmalloc_objs() in rocket_job_open(). If the allocations fail, the code now returns -ENOMEM to prevent using null pointers. > 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 isn't a bug introduced by this patch, but can rocket_acquire_object_fences() fail after drm_sched_job_arm() is called? In rocket_job_push(), a job is armed via drm_sched_job_arm(), which allocates its fence sequence numbers. Immediately after, a fallible operation is called: 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; ... } If rocket_acquire_object_fences() fails, the job is aborted without being pushed to the ring. Does this violate the DRM scheduler contract where drm_sched_job_arm() is a point of no return? Aborting the job after it is called creates permanent sequence number gaps. [ ... ] > @@ -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 is a pre-existing issue, but does rocket_job_open() leak the scheds array on success when the device only has 1 scheduler? When rdev->num_cores == 1, drm_sched_entity_init() intentionally does not store the sched_list pointer in the entity. rocket_job_open() then returns success without saving or freeing the scheds pointer. Additionally, in rocket_job_close(), the cleanup path calls kfree(entity->sched_list), which is a no-op when it is NULL, leaving the array permanently leaked. [Severity: Critical] This isn't a bug introduced by this patch, but can the cleanup path trigger a NULL pointer dereference? If a user submits a job with a malformed parameter and rocket_ioctl_submit_job() fails early during rocket_copy_tasks(), rocket_job_put() cleans up the partially initialized job. At this stage, job->domain is still NULL. rocket_job_cleanup() unconditionally calls the put function: rocket_job_cleanup() { ... rocket_iommu_domain_put(job->domain); ... } Which then dereferences the pointer without a NULL check: rocket_iommu_domain_put() { kref_put(&domain->kref, rocket_iommu_domain_destroy); } Should rocket_job_cleanup() or rocket_iommu_domain_put() verify the domain pointer is valid before attempting to drop the reference? [Severity: High] This is a pre-existing issue, but does rocket_ioctl_submit() silently drop failed jobs? If multiple jobs are submitted and one fails to process due to invalid BO handles or memory allocation failures: rocket_ioctl_submit() { ... for (i = 0; i < args->job_count; i++) rocket_ioctl_submit_job(dev, file, &jobs[i]); ... } The loop entirely ignores the return value of rocket_ioctl_submit_job(), skips the failed job, and continues. The ioctl ultimately returns 0 to userspace, which might leave userspace falsely believing all jobs were successfully queued. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
