Re: [PATCH v3 2/3] accel/rocket: Collect job dependencies before arming

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing read fence attachment for input BOs can lead to data races (Write-after-Read hazards).
- [High] Duplicate GEM objects in job submission arrays cause the IOCTL to fail with -EALREADY.
--

commit bd2a88e8890e01dc0085ebd07568e63dc8dda773
Author: MoGGuU <Naixumogu@whut.edu.cn>

accel/rocket: Collect job dependencies before arming

This commit moves the job dependency collection to occur before the scheduler
job is armed and the scheduler lock is taken. This ensures that memory
allocation failures during dependency collection do not leave the armed job in
an invalid state that cannot be safely aborted.

Cc: stable@vger.kernel.org
Tested-by: Sidong Yang <sidong.yang@furiosa.ai>
Signed-off-by: MoGGuU <Naixumogu@whut.edu.cn>

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index e6052d1973afa..b55e12aecfe64 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -206,19 +206,21 @@ static int rocket_job_push(struct rocket_job *job)
>  	if (ret)
>  		goto err;

[Severity: High]
This is a pre-existing issue, but does the array construction preceding this 
error check allow userspace to trigger an -EALREADY error? 

Looking at the setup before this point in rocket_job_push():

    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 *));

    ret = drm_gem_lock_reservations(bos, bo_count, &acquire_ctx);

If a userspace client submits an in-place inference job where the same BO is
used as both an input and an output (or passed multiple times as input), the
bos array will contain duplicates. When drm_gem_lock_reservations() attempts
to lock the same reservation object twice with the same context, it fails
with -EALREADY, breaking the API and denying valid requests.

> +	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);
>  
[ ... ]
>  
>  		drm_sched_entity_push_job(&job->base);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but are the input BOs left
unsynchronized here?

Looking further down in rocket_job_push(), the completion fence is attached
only to the output BOs:

    rocket_attach_object_fences(job->out_bos, job->out_bo_count, 
                                job->inference_done_fence);

Because the input BOs are entirely omitted, if an unprivileged userspace
client submits an inference job with input buffers, then immediately submits a
job to another engine (like a GPU) to write to those same buffers, can a
Write-after-Read hazard occur? It seems the inference_done_fence isn't
attached to the in_bos reservation objects to protect the read, leading to
potential data corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828050805.38548-1-Naixumogu@whut.edu.cn?part=2