[PATCH] accel/rocket: initialize job domain before cleanup paths

[PATCH] accel/rocket: initialize job domain before cleanup paths

[Shuvam Pandey]

rocket_ioctl_submit_job() releases rjob through rocket_job_put() on
allocation error paths. rocket_job_cleanup() unconditionally calls
rocket_iommu_domain_put(job->domain), but job->domain is assigned only
after task copying and BO lookups. A failure before that assignment can
therefore clean up a job with a NULL domain pointer.

Take the per-file domain reference before the first error path can release
rjob. Also clear rjob->tasks after freeing it in rocket_copy_tasks(), so
the common cleanup path cannot free the task array again after a task-copy
error.

Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Shuvam Pandey <shuvampandey1@gmail.com>
---
 drivers/accel/rocket/rocket_job.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 2f1861f960cc..2b7222afc197 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -102,6 +102,7 @@ rocket_copy_tasks(struct drm_device *dev,
 
 fail:
 	kvfree(rjob->tasks);
+	rjob->tasks = NULL;
 	return ret;
 }
 
@@ -548,6 +549,7 @@ static int rocket_ioctl_submit_job(struct drm_device *dev, struct drm_file *file
 	kref_init(&rjob->refcount);
 
 	rjob->rdev = rdev;
+	rjob->domain = rocket_iommu_domain_get(file_priv);
 
 	ret = drm_sched_job_init(&rjob->base,
 				 &file_priv->sched_entity,
@@ -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);
 	if (ret)
 		goto out_cleanup_job;

Re: [PATCH] accel/rocket: initialize job domain before cleanup paths

[sashiko-bot]

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 <shuvampandey1@gmail.com>

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/6a454b48.6a8fa39a.27019b.984b@mx.google.com?part=1