[PATCH v6 0/2] accel/rocket: Fix allocation handling and scheduler array leak

[PATCH v6 0/2] accel/rocket: Fix allocation handling and scheduler array leak

[Triet Hoang]

This small series addresses allocation and memory management issues in 
the accel/rocket driver:

1. Patch 1 ensures proper allocation checks before using pointers 
   in rocket_job_open()
2. Patch 2 fixes an unreachable memory leak of the scheduler array when 
   running on single-core setups (e.g., num_cores == 1), where 
   drm_sched_entity_init() drops the array pointer.

Link to v5: https://lore.kernel.org/all/20260818041505.1579320-1-triet.hoang.dev@gmail.com/

Changes in v6:
- Rebase on top of drm-misc-next (base commit 5052ea2e6bfc43c64b851c596d3675d17e05e631)
- Addressed review feedback from previous iterations.
- Added Fixes and Tested-by tags where applicable.

Triet Hoang (2):
  drm/rocket: Check allocations before use
  drm/rocket: Keep scheduler allocation in rocket_file_priv

 drivers/accel/rocket/rocket_drv.h |  1 +
 drivers/accel/rocket/rocket_job.c | 13 +++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.53.0

[PATCH v6 1/2] accel/rocket: Check allocations before use

[Triet Hoang]

Check the result of kmalloc_objs() in rocket_job_open() 
before using the allocated buffers.

Also replace the WARN_ON(ret) check in rocket_job_open() with a plain if
since ret can be -ENOMEM from drm_sched_entity_stats_new(). 
That is an allocation failure, not a driver bug, so a plain if 
seems better than a stack trace.

Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
Tested-by: Igor Paunovic <royalnet026@gmail.com>
Tested-by: Sidong Yang <sidong.yang@furiosa.ai>
---
Changes in v2:
- Free scheds when drm_sched_entity_init() fails.
- Initialize ret to 0.

Changes in v3:
- Move patch version descriptions below the '---' marker.

Changes in v4:
- Remove unnecessary initialization of ret to 0.
- Adjust commit message word wrapping.
- Add Fixes tag.

Changes in v5:
- Add check overflow before kvmalloc_array() in rocket_job_push().

Changes in v6:
- Rebase on top of drm-misc-next, remove the rocket_job_push() hunk.
- Change WARN_ON(ret) to plain if check in rocket_job_open().
- Add Tested-by tags.
---
 drivers/accel/rocket/rocket_job.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index f40435505818..704a15513179 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -516,6 +516,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 	unsigned int core;
 	int ret;
 
+	if (!scheds)
+		return -ENOMEM;
+
 	for (core = 0; core < rdev->num_cores; core++)
 		scheds[core] = &rdev->cores[core].sched;
 
@@ -523,10 +526,10 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 				    DRM_SCHED_PRIORITY_NORMAL,
 				    scheds,
 				    rdev->num_cores, NULL);
-	if (WARN_ON(ret))
-		return ret;
+	if (ret)
+		kfree(scheds);
 
-	return 0;
+	return ret;
 }
 
 void rocket_job_close(struct rocket_file_priv *rocket_priv)
-- 
2.53.0

Re: [PATCH v6 1/2] accel/rocket: Check allocations before use

[sashiko-bot]

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

Pre-existing issues:
- [High] Silent failure when submitting multiple jobs due to ignored return value.
- [High] Violation of DRM scheduler contract by calling drm_sched_job_cleanup() on an armed job.
--

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

accel/rocket: Check allocations before use

Check the result of kmalloc_objs() in rocket_job_open() before using the
allocated buffers. It also updates the error handling when
drm_sched_entity_init() fails by replacing WARN_ON(ret) with a plain if.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index f404355058185..ffa0f8db5da65 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -516,6 +516,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  	unsigned int core;
>  	int ret;
>  
> +	if (!scheds)
> +		return -ENOMEM;
> +

[ ... ]

While reviewing this code, I noticed a couple of pre-existing issues in this
file.

[Severity: High]
This is a pre-existing issue, but does calling drm_sched_job_arm() before
rocket_acquire_object_fences() violate the DRM scheduler contract if
resource acquisition fails?

In rocket_job_push():

	scoped_guard(mutex, &rdev->sched_lock) {
		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() returns an error (e.g., due to -ENOMEM in
dma_resv_reserve_fences), the error path aborts the armed job by calling
drm_sched_job_cleanup() back in rocket_ioctl_submit_job().

Since drm_sched_job_arm() initializes fences and assigns a sequence number,
aborting it afterward leaks the assigned sequence number and can corrupt the
scheduler's fence ordering, leading to hangs or crashes in the DRM scheduler
core.


[Severity: High]
This is a pre-existing issue, but is it intentional that the return value of
rocket_ioctl_submit_job() is ignored?

In rocket_ioctl_submit():

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

exit:
	kvfree(jobs);

	return ret;

If rocket_ioctl_submit_job() fails (e.g., due to an invalid BO handle or
-ENOMEM), the failure is silently swallowed and not propagated.

The ioctl will return success even when jobs were not queued, which can cause
userspace clients waiting on implicit fences for these jobs to hang
indefinitely.

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

Re: [PATCH v6 1/2] accel/rocket: Check allocations before use

[Igor Paunovic]

Hi Triet,

Re-ran v6 as posted, on its drm-misc-next base (5052ea2e6bfc), on
RK3588 (Orange Pi 5 Plus, three cores) with PROVE_LOCKING and
DEBUG_ATOMIC_SLEEP: 30 sequential clients, 10 pairs of concurrent
clients and 5 after runtime suspend, 55 inferences all bit-correct
against the reference, no warning or lockdep report, cores back in
runtime suspend afterwards. Same limit as on v5: the allocation
failure branches were reviewed, not fault-injected. The tag stands
for v6; please keep its comment:

Tested-by: Igor Paunovic <royalnet026@gmail.com> # RK3588, no fault injection

Regards,
Igor

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

[Triet Hoang]

The scheduler array passed to drm_sched_entity_init() is retained by the
entity when the device has multiple cores. drm_sched_entity_init() stores 
the list conditionally:

	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;

With one scheduler, it keeps only entity->rq and drops the array pointer
while rocket_job_open() allows its local copy to go out of scope. So
for num_cores == 1 the kfree() in rocket_job_close() is a kfree(NULL)
and the array leaks unreachably. For num_cores > 1 there is no leak,
which is why this is invisible in normal use on RK3588.

Keep the allocation in rocket_file_priv so rocket_job_close() can destroy
the entity before freeing the array. Free the array directly if entity
initialization fails, since rocket_job_open() does not call
rocket_job_close() on that error path.

Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")

Suggested-by: Igor Paunovic <royalnet026@gmail.com>
Link: https://lore.kernel.org/all/20260817093009.22359-1-royalnet026@gmail.com/
Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
Tested-by: Igor Paunovic <royalnet026@gmail.com>
Tested-by: Sidong Yang <sidong.yang@furiosa.ai>
---
Changes in v5:
- Free rocket_priv->scheds instead of entity->sched_list in rocket_job_close().

Changes in v6:
- Add Tested-by and Fixes tags.
- Move kfree() below the drm_sched_entity_destroy() call in rocket_job_close().
- Update commit message to explain why the scheduler array must outlive the scheduler entity.
---
 drivers/accel/rocket/rocket_drv.h | 1 +
 drivers/accel/rocket/rocket_job.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/rocket/rocket_drv.h b/drivers/accel/rocket/rocket_drv.h
index 2c673bb99ccc..9421e48ec5d8 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 704a15513179..f27e6264f91d 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;
 }
@@ -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);
 }
 
 int rocket_job_is_idle(struct rocket_core *core)
-- 
2.53.0

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

[sashiko-bot]

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

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

[Igor Paunovic]

Hi Triet,

Same run as on 1/2, on v6 as posted: ten pairs of concurrent clients,
so two rocket_file_priv instances with their own scheduler entities
alive and submitting at the same time, then closed, with the kfree()
now after drm_sched_entity_destroy(). All output bit-correct, no
lockdep or atomic-sleep report, clean autosuspend afterwards. The tag
stands for v6; please keep its comment:

Tested-by: Igor Paunovic <royalnet026@gmail.com> # RK3588, concurrent clients

Regards,
Igor