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

[sashiko-bot@kernel.org]

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