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

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

[MoGGuU]

Several error paths in the job submission path can be triggered by
unprivileged userspace through malformed DRM_ROCKET_SUBMIT requests.

First, the input and output BO handle counts are __u32, but GEM lookup and
reservation helpers take int counts.  Values outside the signed range can
become negative.  A combined count above INT_MAX can also overflow at the
call sites.  Reject counts that cannot be represented safely before looking
up the BOs.

Second, rocket_job_push() arms the scheduler job before collecting its
implicit dependencies.  Dependency collection can fail with -ENOMEM, but
drm_sched_job_arm() is a point of no return.  An armed job must be pushed;
it must not be aborted with drm_sched_job_cleanup().  Collect dependencies
before taking the scheduler lock and arming the job.  Only operations that
cannot fail remain after arm().

Finally, rocket_ioctl_submit() discards each job's return value and reports
success even when every job fails.  Return the first error
and stop submitting the remaining jobs.  Jobs queued before an error remain
queued, giving the ioctl ordered partial-submit semantics.

Tested on RK3588 with zero task counts, invalid task pointers, invalid BO
handles, and oversized BO counts.  The requests returned the expected error
codes without warnings or errors in the kernel log.

Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: MoGGuU <Naixumogu@whut.edu.cn>
---
 drivers/accel/rocket/rocket_job.c | 34 +++++++++++++++++++------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index bb77b6bf0..c42bbf486 100644
--- 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;
+
+	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);
 	}
 
@@ -556,6 +555,12 @@ static int rocket_ioctl_submit_job(struct drm_device *dev, struct drm_file *file
 	if (job->task_count == 0)
 		return -EINVAL;
 
+	/* GEM lookup and reservation helpers take signed object counts. */
+	if (job->in_bo_handle_count > INT_MAX ||
+	    job->out_bo_handle_count > INT_MAX ||
+	    job->in_bo_handle_count > INT_MAX - job->out_bo_handle_count)
+		return -EINVAL;
+
 	rjob = kzalloc_obj(*rjob);
 	if (!rjob)
 		return -ENOMEM;
@@ -639,8 +644,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);

base-commit: d85a5e9dfb42449d9f37b0ddc6ec30b129f481ce
-- 
2.43.0

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

[sashiko-bot]

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

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

[MoGGuU]

Thanks for the review.

I checked the reported partial-submission restart path.  I do not believe
the new issue is reachable as described.  The interruptible operation in
this path is drm_gem_lock_reservations().  Its
dma_resv_lock_interruptible() call returns -EINTR when interrupted, as
documented by ww_mutex_lock_interruptible(), and drm_ioctl() propagates
that value unchanged.  It does not return an -ERESTART* value, so the
kernel will not automatically restart this ioctl from job zero.  The other
failure paths in rocket_ioctl_submit_job() also do not return an
-ERESTART* value.  Userspace could still choose to retry an EINTR manually;
the ordered partial-submit semantics documented in the commit message mean
that it must not blindly retry the whole batch.

The duplicate-BO report does not cause a double unlock either.  Trying to
lock the same WW mutex twice with the same acquisition context returns
-EALREADY.  drm_gem_lock_reservations() then unlocks the reservations it
already acquired and finalizes the context before returning the error.
rocket_job_push() takes its err path in that case, not err_unlock.  I also
verified this on RK3588: a submission containing the same input BO handle
twice returned EALREADY without a new warning or error in the kernel log.

The two dma-resv fence observations are valid pre-existing issues.  The
completion fence should be attached with DMA_RESV_USAGE_READ to input BOs
and DMA_RESV_USAGE_WRITE to output BOs after drm_sched_job_arm() creates
the finished fence, but before drm_sched_entity_push_job() makes the job
runnable.  I will address those in a separate follow-up patch so this patch
remains focused on its stated error-path fixes.

-- 
MoGGuU