Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Returning -ERESTARTSYS after partial job submission causes duplicate job execution upon syscall restart. Pre-existing issues: - [High] Fences are attached to the reservation objects after the job has been pushed to the scheduler. - [High] The job fails to attach a read fence to its input BOs, causing data races with subsequent writers. - [High] Passing duplicate BO handles causes a double-unlock of the GEM object's WW mutex, leading to a kernel warning and potential lock state corruption. -- --- Patch [1]: [PATCH] accel/rocket: Fix job submit error handling --- commit 6dff53fbca0f674f61df49145b960506e0f490e8 Author: MoGGuU <[email protected]> accel/rocket: Fix job submit error handling This commit fixes several job submission error paths. It validates BO handle counts to prevent integer overflows, moves dependency collection before arming the scheduler job, and aborts submission on the first error instead of continuing silently. > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -206,21 +206,20 @@ static int rocket_job_push(struct rocket_job *job) > if (ret) > goto err; > > + 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 jumping to err_unlock here cause a double-unlock if there are duplicate BOs? Earlier in rocket_job_push(), the bos array is built by copying in_bos and out_bos without deduplication. When drm_gem_unlock_reservations() is called in the err_unlock path, it assumes all elements in the array are unique. Could passing duplicate handles corrupt the WW mutex state? > + > + ret = rocket_acquire_object_fences(job->out_bos, job->out_bo_count, > + &job->base, true); > + if (ret) > + goto err_unlock; > + > scoped_guard(mutex, &rdev->sched_lock) { > 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; > - > - ret = rocket_acquire_object_fences(job->out_bos, > job->out_bo_count, &job->base, true); > - if (ret) > - goto err_unlock; > - > kref_get(&job->refcount); /* put by scheduler job completion */ > - > drm_sched_entity_push_job(&job->base); > } > > rocket_attach_object_fences(job->out_bos, job->out_bo_count, > job->inference_done_fence); [Severity: High] This is a pre-existing issue, but is it safe to attach fences after pushing the job to the scheduler? The code executes drm_sched_entity_push_job() before calling rocket_attach_object_fences(). Can lockless RCU readers observe an idle buffer before the fences are attached, resulting in unprotected concurrent access? [Severity: High] This is also a pre-existing issue, but it looks like fences are only attached to the output BOs here. Since the function omits passing job->in_bos to rocket_attach_object_fences(), won't the input BOs lack read fences? Could this allow subsequent writers to proceed without waiting for this read operation to finish, leading to data corruption? [ ... ] > @@ -640,8 +645,11 @@ int rocket_ioctl_submit(struct drm_device *dev, void > *data, struct drm_file *fil > } > > > - for (i = 0; i < args->job_count; i++) > - rocket_ioctl_submit_job(dev, file, &jobs[i]); > + for (i = 0; i < args->job_count; i++) { > + ret = rocket_ioctl_submit_job(dev, file, &jobs[i]); > + if (ret) > + goto exit; > + } > > exit: > kvfree(jobs); [Severity: High] What happens if the submission is interrupted and returns -ERESTARTSYS? If an error occurs after a few jobs have already been successfully submitted, the ioctl aborts and returns the error code directly. Since previously queued jobs remain intact, wouldn't a syscall restart (from index 0) force duplicate execution of those already-submitted jobs? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
