Re: [PATCH] accel/rocket: Fix job submit error handling

[sashiko-bot@kernel.org]

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 <Naixumogu@whut.edu.cn>

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/20260813142059.151644-1-Naixumogu@whut.edu.cn?part=1