Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing NULL check after `kvmalloc_array()` leads to an immediate kernel panic via `memcpy()`. - [High] Calling `drm_sched_job_cleanup()` on an already-armed scheduler job violates the DRM scheduler API contract and leaks a sequence number. - [High] Failure to attach the execution fence to input BOs causes a read-write data race. - [High] Early error returns in `rocket_job_run()` leak PM runtime usage counters and return an unsignaled fence, hanging the DRM scheduler. -- commit 6d28e47f76bcafb92f750ef15d6a214c957e5c54 Author: Shuvam Pandey <[email protected]> accel/rocket: initialize job domain before cleanup paths This patch moves the job domain initialization earlier in the job submission process to prevent a NULL pointer dereference during error cleanup. It also clears the task array pointer after freeing it to avoid double-free issues. > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index 2f1861f960ccb..2b7222afc1975 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c [ ... ] > @@ -573,8 +575,6 @@ static int rocket_ioctl_submit_job(struct drm_device > *dev, struct drm_file *file > > rjob->out_bo_count = job->out_bo_handle_count; > > - rjob->domain = rocket_iommu_domain_get(file_priv); > - > ret = rocket_job_push(rjob); [Severity: Critical] This is a pre-existing issue, but can this cause a kernel panic? Looking at rocket_job_push(), if kvmalloc_array() fails and returns NULL, would the subsequent memcpy() try to write to a NULL destination? rocket_job_push(): bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void *), GFP_KERNEL); memcpy(bos, job->in_bos, job->in_bo_count * sizeof(void *)); [Severity: High] This isn't a bug introduced by this patch, but does this violate the DRM scheduler API contract? In rocket_job_push(), drm_sched_job_arm() is called before the fallible rocket_acquire_object_fences(). If fence acquisition fails, does cleaning up the already-armed job leave a gap in the fence timeline sequence? rocket_job_push(): drm_sched_job_arm(&job->base); ... ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, &job->base, false); if (ret) goto err_unlock; [Severity: High] This is a pre-existing issue, but does this cause a read-write data race? In rocket_job_push(), rocket_attach_object_fences() is only called for out_bos. Should the inference_done_fence also be attached to in_bos as a read dependency so subsequent jobs don't write to them while they are still being read? rocket_job_push(): rocket_attach_object_fences(job->out_bos, job->out_bo_count, job->inference_done_fence); [Severity: High] This isn't a bug introduced by this patch, but does this early return in rocket_job_run() leak the PM runtime usage counter and hang the scheduler? If pm_runtime_get_sync() fails, it still increments the runtime PM counter. Does returning an unsignaled fence instead of an error pointer cause the DRM scheduler to wait indefinitely? rocket_job_run(): ret = pm_runtime_get_sync(core->dev); if (ret < 0) return fence; > if (ret) > goto out_cleanup_job; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
