Re: [PATCH v6 2/2] drm/rocket: Keep scheduler allocation in rocket_file_priv

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race condition in rocket_job_open() leading to kernel panic via ZERO_SIZE_PTR dereference.
- [High] The ioctl submit handler aborts an armed scheduler job if fence acquisition fails, violating the drm_sched_job_arm() API contract.
- [High] Unbalanced iommu_group reference acquisition causes a resource leak in the interrupt handler.
--

commit 3814d4c84d6491257e9ab5433448c08da2327e1e
Author: Triet Hoang <triet.hoang.dev@gmail.com>

drm/rocket: Keep scheduler allocation in rocket_file_priv

The scheduler array passed to drm_sched_entity_init() is retained by the
entity when the device has multiple cores. This commit fixes a memory leak
for single-core devices by keeping the allocation in rocket_file_priv.

Link: https://lore.kernel.org/all/20260817093009.22359-1-royalnet026@gmail.com/

> diff --git a/drivers/accel/rocket/rocket_drv.h b/drivers/accel/rocket/rocket_drv.h
> index 2c673bb99ccc1..9421e48ec5d84 100644
> --- a/drivers/accel/rocket/rocket_drv.h
> +++ b/drivers/accel/rocket/rocket_drv.h
> @@ -23,6 +23,7 @@ struct rocket_file_priv {
>  	struct drm_mm mm;
>  	struct mutex mm_lock;
>  
> +	struct drm_gpu_scheduler **scheds;
>  	struct drm_sched_entity sched_entity;
>  };
>  
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index ffa0f8db5da65..ffb3cf5684c1e 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -528,6 +528,8 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  				    rdev->num_cores, NULL);
>  	if (ret)
>  		kfree(scheds);
> +	else
> +		rocket_priv->scheds = scheds;
>  
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
rdev->num_cores to change concurrently during rocket_job_open()?

If userspace rapidly opens the DRM device while the driver is still probing
cores, could rdev->num_cores increase between the allocation and the loop?

drivers/accel/rocket/rocket_job.c:rocket_job_open() {
    ...
	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
							 rdev->num_cores);
    ...
	for (core = 0; core < rdev->num_cores; core++)
		scheds[core] = &rdev->cores[core].sched;
    ...
}

If rdev->num_cores changes from 0 to 1 after the allocation, kmalloc_objs()
will return ZERO_SIZE_PTR, and the loop will immediately write to scheds[0],
causing a kernel panic.

> @@ -536,8 +538,8 @@ void rocket_job_close(struct rocket_file_priv *rocket_priv)
>  {
>  	struct drm_sched_entity *entity = &rocket_priv->sched_entity;
>  
> -	kfree(entity->sched_list);
>  	drm_sched_entity_destroy(entity);
> +	kfree(rocket_priv->scheds);
>  }

[Severity: High]
This is a pre-existing issue, but does the error handling in rocket_job_push()
violate the drm_sched_job_arm() API contract?

drivers/accel/rocket/rocket_job.c: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 after the job is armed, the function
bails out without calling drm_sched_entity_push_job(). 

The submit ioctl then cleans up the armed job:

drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit_job() {
    ...
	ret = rocket_job_push(rjob);
	if (ret)
		goto out_cleanup_job;

out_cleanup_job:
	if (ret)
		drm_sched_job_cleanup(&rjob->base);
    ...
}

Is it safe to call drm_sched_job_cleanup() on a job that has already been
armed? The DRM scheduler API requires that drm_sched_job_arm() is the point
of no return and must be followed by drm_sched_entity_push_job(). Aborting
here could leave armed fences in an inconsistent state.

[Severity: High]
This is a pre-existing issue, but is there a leaked IOMMU group reference
here?

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
    ...
			iommu_detach_group(NULL, iommu_group_get(core->dev));
			dma_fence_signal(core->in_flight_job->done_fence);
    ...
}

Every time a hardware job completes, iommu_group_get() is called to acquire
a reference, which is then passed to iommu_detach_group(). Since
iommu_detach_group() does not consume the reference and iommu_group_put() is
never called, doesn't this leak one reference per job completion?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914045605.381933-1-triet.hoang.dev@gmail.com?part=2