Re: [PATCH v3] drm/rocket: Check allocations before use

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

Pre-existing issues:
- [Critical] Critical NULL pointer dereference in `rocket_job_cleanup()` leading to kernel panic.
- [High] Memory leak of `scheds` array in `rocket_job_open` on success.
- [High] Fallible operations placed after `drm_sched_job_arm()` violate the strict DRM scheduler contract.
- [High] Ignored return value in `rocket_ioctl_submit()` silently swallows failed job submissions.
--

--- Patch [1]: [PATCH v3] drm/rocket: Check allocations before use ---
commit e005f1f8daeacc5c010970453e21490c444073da
Author: Triet Hoang <[email protected]>

drm/rocket: Check allocations before use

This commit adds missing allocation failure checks for kvmalloc_array() in
rocket_job_push() and kmalloc_objs() in rocket_job_open(). If the
allocations fail, the code now returns -ENOMEM to prevent using null
pointers.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index ac51bff39833f..24e1a61f71e7b 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -192,6 +192,9 @@ static int rocket_job_push(struct rocket_job *job)
>  
>  	bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void *),
>  			     GFP_KERNEL);
> +	if (!bos)
> +		return -ENOMEM;
> +
>  	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 *));

[Severity: High]
This isn't a bug introduced by this patch, but can
rocket_acquire_object_fences() fail after drm_sched_job_arm() is called?

In rocket_job_push(), a job is armed via drm_sched_job_arm(), which
allocates its fence sequence numbers. Immediately after, a fallible
operation is called:

rocket_job_push() {
        ...
        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;
        ...
}

If rocket_acquire_object_fences() fails, the job is aborted without being
pushed to the ring. Does this violate the DRM scheduler contract where
drm_sched_job_arm() is a point of no return? Aborting the job after it is
called creates permanent sequence number gaps.

[ ... ]

> @@ -499,7 +502,10 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
>  							 rdev->num_cores);
>  	unsigned int core;
> -	int ret;
> +	int ret = 0;
> +
> +	if (!scheds)
> +		return -ENOMEM;
>  
>  	for (core = 0; core < rdev->num_cores; core++)
>  		scheds[core] = &rdev->cores[core].sched;
> @@ -509,9 +515,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  				    scheds,
>  				    rdev->num_cores, NULL);
>  	if (WARN_ON(ret))
> -		return ret;
> +		kfree(scheds);
>  
> -	return 0;
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does rocket_job_open() leak the scheds
array on success when the device only has 1 scheduler?

When rdev->num_cores == 1, drm_sched_entity_init() intentionally does not
store the sched_list pointer in the entity. rocket_job_open() then returns
success without saving or freeing the scheds pointer.

Additionally, in rocket_job_close(), the cleanup path calls
kfree(entity->sched_list), which is a no-op when it is NULL, leaving the
array permanently leaked.

[Severity: Critical]
This isn't a bug introduced by this patch, but can the cleanup path trigger
a NULL pointer dereference?

If a user submits a job with a malformed parameter and
rocket_ioctl_submit_job() fails early during rocket_copy_tasks(),
rocket_job_put() cleans up the partially initialized job. At this stage,
job->domain is still NULL.

rocket_job_cleanup() unconditionally calls the put function:

rocket_job_cleanup() {
        ...
        rocket_iommu_domain_put(job->domain);
        ...
}

Which then dereferences the pointer without a NULL check:

rocket_iommu_domain_put() {
        kref_put(&domain->kref, rocket_iommu_domain_destroy);
}

Should rocket_job_cleanup() or rocket_iommu_domain_put() verify the domain
pointer is valid before attempting to drop the reference?

[Severity: High]
This is a pre-existing issue, but does rocket_ioctl_submit() silently drop
failed jobs?

If multiple jobs are submitted and one fails to process due to invalid BO
handles or memory allocation failures:

rocket_ioctl_submit() {
        ...
        for (i = 0; i < args->job_count; i++)
                rocket_ioctl_submit_job(dev, file, &jobs[i]);
        ...
}

The loop entirely ignores the return value of rocket_ioctl_submit_job(),
skips the failed job, and continues. The ioctl ultimately returns 0 to
userspace, which might leave userspace falsely believing all jobs were
successfully queued.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1