[PATCH v2] drm/rocket: Check allocations before use

[PATCH v2] drm/rocket: Check allocations before use

[Triet Hoang]

Check the result of kvmalloc_array() in rocket_job_push() and
kmalloc_objs() in rocket_job_open() before using
the allocated buffer.

Changes in v2:
- Free scheds when drm_sched_entity_init() fails.
- Initialize ret to 0.

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
 drivers/accel/rocket/rocket_job.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39833..24e1a61f71e7 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 *));
 
@@ -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;
 }
 
 void rocket_job_close(struct rocket_file_priv *rocket_priv)
-- 
2.53.0

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

[Markus Elfring]

> Check the result of kvmalloc_array() in rocket_job_push() and
> kmalloc_objs() in rocket_job_open() before using
> the allocated buffer.

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n669
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n145
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2-rc7#n34


> Changes in v2:
…
> ---
>  drivers/accel/rocket/rocket_job.c | 12 +++++++++---
…

Please move patch version descriptions behind the marker line.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n795

Regards,
Markus

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

[Triet Hoang]

Check the result of kvmalloc_array() in rocket_job_push() and
kmalloc_objs() in rocket_job_open() before using
the allocated buffer.

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
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.

 drivers/accel/rocket/rocket_job.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39833..24e1a61f71e7 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 *));
 
@@ -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;
 }
 
 void rocket_job_close(struct rocket_file_priv *rocket_priv)
-- 
2.53.0

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

[Markus Elfring]

> Check the result of kvmalloc_array() in rocket_job_push() and
> kmalloc_objs() in rocket_job_open() before using
> the allocated buffer.

* Would an other word wrap variant be nicer here?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n669

* How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n145
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2-rc7#n34


Regards,
Markus

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

[Triet Hoang]

Hi Markus,

Thanks for the review. I'll update the patch accordingly in the next version.

Regards,
Triet

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

[Igor Paunovic]

Hi Triet,

Thanks for picking this up -- the rocket driver has few enough eyes on it
that allocation-check patches are welcome.

The rocket_job_push() half looks right to me. The early return skips the
err: label, but bos is NULL there anyway, and the caller
(rocket_ioctl_submit_job()) takes the goto out_cleanup_job path, which
does drm_sched_job_cleanup() and rocket_job_put(). Nothing is leaked and
nothing is armed yet, so returning early is safe.

On rocket_job_open(), the v2 change fixes the error path, but I think it
only covers half of what was reported. The other half is still there:
the array leaks on a single-core device even when nothing fails.

drm_sched_entity_init() stores the caller's array only when it will
actually need it:

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

(drivers/gpu/drm/scheduler/sched_entity.c, unchanged in current
mainline). And rocket_job_close() frees exactly that field:

	kfree(entity->sched_list);
	drm_sched_entity_destroy(entity);

So when rdev->num_cores == 1, drm_sched_entity_init() succeeds,
entity->sched_list is NULL, rocket_job_close() frees NULL, and the array
that rocket_job_open() allocated is never freed. One pointer per open(),
unbounded across open/close cycles.

That is not a hypothetical configuration. The RK3576 series currently on
the list enables exactly one core on the ROCK 4D -- its commit message
says so in as many words ("Only rknn_core_0 is enabled: the driver binds
one core per node and the second core is left to whoever can test it").
Any RK3588 DT that leaves a single core enabled lands in the same place.

I would suggest not depending on drm_sched_entity_init()'s internal
choice about sched_list at all: keep the pointer in rocket_file_priv and
free it unconditionally in rocket_job_close(). That covers one core and
many cores with the same line, and it stops rocket_job_close() from
reaching into a scheduler-internal field to decide what it owns. But
that is a bigger change than the one you set out to make, so it may be
better as a separate patch -- Tomeu's call.

Two smaller things:

  - With the check added, ret is assigned unconditionally from
    drm_sched_entity_init(), so the "int ret = 0" initialiser in v2 is
    no longer doing anything.

  - Heads-up on collision: I have a patch in flight that touches these
    same lines of rocket_job_open() ("accel/rocket: keep core slots
    stable across unbind and rebind", part of a two-patch lifecycle
    series). Whichever of us lands first, the other rebases -- I am happy
    for that to be me. Worth mentioning because in my version the count
    passed to drm_sched_entity_init() is the number of *live* cores, so
    once cores are unbound down to one, the leak above starts happening
    on a multi-core board too, at runtime.

I have not run your patch, so no tag from me. If it would help, I can
test it on RK3588 with three cores and again with two of them unbound,
and check the single-core case with kmemleak.

Regards,
Igor

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

[Triet Hoang]

From: Triet Hoang <triet.hoang.dev@gmail.com>

Hi Igor,

Thanks for the suggestion. I agree that keeping the allocation in rocket_file_priv 
would make the ownership clearer. I will implement this idea as a seperate patch.

Best Regards,
Triet

[PATCH v4 1/2] drm/rocket: Check allocations before use

[Triet Hoang]

From: Triet Hoang <triet.hoang.dev@gmail.com>

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

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

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
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.

 drivers/accel/rocket/rocket_job.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39833..adcc792541ec 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 *));
 
@@ -501,6 +504,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;
 
@@ -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;
 }
 
 void rocket_job_close(struct rocket_file_priv *rocket_priv)
-- 
2.53.0

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

[Triet Hoang]

From: Triet Hoang <triet.hoang.dev@gmail.com>

Keep the scheduler allocation in rocket_file_priv
and free it unconditionally in rocket_job_close().

Suggested-by: Igor Paunovic <royalnet026@gmail.com>
Link: https://lore.kernel.org/all/20260817093009.22359-1-royalnet026@gmail.com/#t
Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
 drivers/accel/rocket/rocket_drv.h | 1 +
 drivers/accel/rocket/rocket_job.c | 4 ++++
 2 files changed, 5 insertions(+)

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 adcc792541ec..ff1d9e802024 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -514,8 +514,11 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 				    DRM_SCHED_PRIORITY_NORMAL,
 				    scheds,
 				    rdev->num_cores, NULL);
+
 	if (WARN_ON(ret))
 		kfree(scheds);
+	else
+		rocket_priv->scheds = scheds;
 
 	return ret;
 }
@@ -525,6 +528,7 @@ void rocket_job_close(struct rocket_file_priv *rocket_priv)
 	struct drm_sched_entity *entity = &rocket_priv->sched_entity;
 
 	kfree(entity->sched_list);
+	kfree(rocket_priv->scheds);
 	drm_sched_entity_destroy(entity);
 }
 
-- 
2.53.0

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

[Markus Elfring]

…
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -514,8 +514,11 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  				    DRM_SCHED_PRIORITY_NORMAL,
>  				    scheds,
>  				    rdev->num_cores, NULL);
> +
>  	if (WARN_ON(ret))
>  		kfree(scheds);
…

Why do you think that an additional blank would be helpful at this place?

Regards,
Markus

Re: [PATCH v4 1/2] drm/rocket: Check allocations before use

[Markus Elfring]

…
> +++ 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);
…

I see opportunities for corresponding collateral evolution.
How do you think about to apply the attribute “__free(kvfree)” also in such
a function implementation by another update step?
https://elixir.bootlin.com/linux/v7.2-rc7/source/include/linux/slab.h#L1420

Regards,
Markus

Re: [PATCH v4 1/2] drm/rocket: Check allocations before use

[Triet Hoang]

> I see opportunities for corresponding collateral evolution.
> How do you think about to apply the attribute “__free(kvfree)” also in such
> a function implementation by another update step?

I am not sure I understand your idea.
Could you explain more? This attribute is a new thing to media

Regards,
Triet

Re: [v4 1/2] drm/rocket: Check allocations before use

[Markus Elfring]

>> I see opportunities for corresponding collateral evolution.
>> How do you think about to apply the attribute “__free(kvfree)” also in such
>> a function implementation by another update step?
> 
> I am not sure I understand your idea.
> Could you explain more? This attribute is a new thing to media
There are programming interfaces supported to some degree
for the application of scope-based resource management.
https://elixir.bootlin.com/linux/v7.2-rc7/source/include/linux/cleanup.h#L10

Would you like to take any adjustment possibilities better into account accordingly?

Regards,
Markus

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

[Triet Hoang]

> > There are programming interfaces supported to some degree
> for the application of scope-based resource management.
> https://elixir.bootlin.com/linux/v7.2-rc7/source/include/linux/cleanup.h#L10
> 
> Would you like to take any adjustment possibilities better into account accordingly?

Thanks for explaining. I am happy to continue working on this, but I think
this would be more of a refactoring to use the new API than a bug fix.
Furthermore, I don't have much experience with the new cleanup API yet, so
I think it would be better to consider this as a separate change in the
future, after these patches have been reviewed and I have gained more
experience with the new API.

Anyway, thanks for suggesting this. I'll take a look at it and keep it in
mind for future changes.

Regards,
Triet

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

[Triet Hoang]

Hi Igor,

After reading the code carefully, I think that we can check 
if entity->sched_list is NULL or not, then we can free the array
that rocket_job_open() allocated.

This will look like 
if (WARN_ON(ret) || !(&rocket_priv->sched_entity->sched_list))
 	kfree(scheds);

How do you think about that

Best Regards,
Triet

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

[Triet Hoang]

> This will look like 
> if (WARN_ON(ret) || !(&rocket_priv->sched_entity->sched_list))
>  	kfree(scheds);
> How do you think about that

Hi Igor,

This is a bad idea. Sorry about that and just forget it.

Regards,
Triet

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

[Triet Hoang]

> This will look like 
> if (WARN_ON(ret) || !(&rocket_priv->sched_entity->sched_list))
>  	kfree(scheds);
> How do you think about that

Hi Igor,

This is a bad idea. Sorry about that and just forget it.

Regards,
Triet

[PATCH v5 1/2] drm/rocket: Check allocations before use

[Triet Hoang]

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

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

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
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().
---
 drivers/accel/rocket/rocket_job.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39833..15e93c355c38 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -189,9 +189,15 @@ static int rocket_job_push(struct rocket_job *job)
 	struct drm_gem_object **bos;
 	struct ww_acquire_ctx acquire_ctx;
 	int ret = 0;
+	size_t bos_count;
+
+	if (check_add_overflow(job->in_bo_count, job->out_bo_count, &bos_count))
+		return -EOVERFLOW;
+
+	bos = kvmalloc_array(bos_count, sizeof(void *), GFP_KERNEL);
+	if (!bos)
+		return -ENOMEM;
 
-	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 *));
 	memcpy(&bos[job->in_bo_count], job->out_bos, job->out_bo_count * sizeof(void *));
 
@@ -501,6 +507,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;
 
@@ -509,9 +518,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;
 }
 
 void rocket_job_close(struct rocket_file_priv *rocket_priv)
-- 
2.53.0

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

[Triet Hoang]

Keep the scheduler allocation in rocket_file_priv
and free it unconditionally in rocket_job_close().

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>
---
Changses in v5:
- Free rocket_priv->scheds instead of entity->sched_list in rocket_job_close().
---
 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 15e93c355c38..50b6e0e06ed8 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -519,6 +519,8 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 				    rdev->num_cores, NULL);
 	if (WARN_ON(ret))
 		kfree(scheds);
+	else
+		rocket_priv->scheds = scheds;
 
 	return ret;
 }
@@ -527,7 +529,7 @@ void rocket_job_close(struct rocket_file_priv *rocket_priv)
 {
 	struct drm_sched_entity *entity = &rocket_priv->sched_entity;
 
-	kfree(entity->sched_list);
+	kfree(rocket_priv->scheds);
 	drm_sched_entity_destroy(entity);
 }
 
-- 
2.53.0

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

[Triet Hoang]

Hi Everyone,

Thanks for the detailed review. This is my first time contributing to the
kernel and using the mailing list, so I really appreciate your feedback.

The thread has become quite long, so I'll leave it here for now and wait
for feedback from Tomeu before making further changes.

> I have not run your patch, so no tag from me. If it would help, I can
> test it on RK3588 with three cores and again with two of them unbound,
> and check the single-core case with kmemleak.

I don't have the hardware to test these cases myself, so it would be very
helpful if you could run the patch (v5) and share any feedback. It would also
help me better understand the behavior of the driver and the testing
process.

Thanks again for taking the time to review and test it.

Regards,
Triet

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

[Igor Paunovic]

Hi Triet,

Same run as on 1/2, with one round aimed specifically at this patch:
ten pairs of concurrent clients, so two rocket_file_priv instances with
their own scheduler entities live at the same time, submitting in
parallel. All pairs completed with bit-correct output, no lockdep or
atomic-sleep hits, and clean autosuspend afterwards.

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

Regards,
Igor

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

[Sidong Yang]

Hi Triet,

The code looks right to me, but the commit message does not say why the
change is needed - and that is what stalled the same change before:

  https://lore.kernel.org/all/20251107024620.912403-1-liqiang01@kylinos.cn/

  > Are you sure of this? The list should be freed in rocket_job_close(),
  > just before freeing the entity.

Fair against that patch, which added kfree(rocket_priv->scheds) while
keeping kfree(entity->sched_list) - a double free for num_cores > 1.
Yours replaces the free instead, so it avoids that.

The missing argument is why kfree(entity->sched_list) is not already
enough. 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,
and rocket_job_open() has already let its own copy 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.

num_cores == 1 is reachable there anyway: rocket_probe() registers the
DRM device when the *first* core binds and increments num_cores as the
others follow, so an open() racing the probe of cores 1 and 2 gets a
single-scheduler entity. rocket_remove() also decrements num_cores with
the device still registered.

Independent of the leak, the driver reads a pointer back out of
drm_sched's own struct field to free it, and what lands in that field is
drm_sched's decision. Owning the allocation removes that dependency -
that seems worth stating on its own.

Two small things:

 - No Fixes: tag, although this fixes a leak. Same one as 1/2 fits.

 - kfree() still runs before drm_sched_entity_destroy(), so
   entity->sched_list dangles across teardown. Not a live UAF today
   (select_rq() is only reached from drm_sched_job_arm(), and .postclose
   runs after the last fd reference is gone), but the order is backwards
   and moving it after destroy costs nothing.

Caveat: I could not measure the leak - no kmemleak or KASAN here, and
with three cores I cannot open the num_cores == 1 window on purpose. The
above is from the source. What I did verify is that this plus the
rocket_job_open() half of 1/2, rebased on drm-misc-next, builds clean and
does not regress normal submits or the malformed inputs I tried.

Nit: prefix should be accel/rocket:, and "Changses" below the ---.

Thanks,
Sidong

Re: [PATCH v5 1/2] drm/rocket: Check allocations before use

[Igor Paunovic]

Hi Triet,

Ran v5 on RK3588 hardware (Orange Pi 5 Plus, all three cores), on a
tree with PROVE_LOCKING and DEBUG_ATOMIC_SLEEP enabled: 30 sequential
client cycles (open/submit/close), 10 pairs of concurrent clients, and
a round across runtime suspend/resume - 45/45 inferences bit-correct
against the reference, zero warnings of any kind, cores returning to
runtime-suspended cleanly.

One honest limit: this exercises the success paths your checks guard;
the allocation-failure branches themselves I reviewed but did not
fault-inject.

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

Regards,
Igor

Re: [PATCH v5 1/2] drm/rocket: Check allocations before use

[Triet Hoang]

Hi Igor,

Thank you so much for testing my code!

Regards,
Triet

Re: [PATCH v5 1/2] drm/rocket: Check allocations before use

[Sidong Yang]

Hi Triet,

This does not apply to drm-misc-next, which is the tree accel/rocket
goes through:

  error: patch failed: drivers/accel/rocket/rocket_job.c:189

That is the rocket_job_push() hunk, already fixed there by

  https://lore.kernel.org/all/20260524155716.90955-1-meatuni001@gmail.com/

with the same NULL check, the same check_add_overflow() and the same
Fixes: tag. Please drop that hunk.

The rocket_job_open() part is still needed. I forced the kmalloc_objs()
there to fail on drm-misc-next (debug-only module param, this kernel has
no fault injection) and open(2) oopses on a ROCK 5B+:

  pc : rocket_job_open+0x64/0xd0 [rocket]
   rocket_open / drm_file_alloc / drm_open_helper / accel_open

With your check, open(2) returns -ENOMEM and the log stays clean. Worth
mentioning in the commit message: rocket_open() calls try_module_get()
before rocket_job_open(), so that oops also leaks the module reference
and rmmod is blocked until reboot ("Module rocket is in use").

kfree(scheds) on the drm_sched_entity_init() error path is right too -
rocket_job_close() never runs if rocket_job_open() fails, since
rocket_open() unwinds and .postclose is not called.

> 	if (WARN_ON(ret))

Since you are touching this line: ret can be -ENOMEM from
drm_sched_entity_stats_new(). That is an allocation failure, not a driver
bug, so a plain if (ret) seems better than a stack trace.

So a v6 would be just the rocket_job_open() change, rebased. I built and
tested that (this patch minus the push hunk, plus 2/2); happy to give a
Tested-by on it.

Nit: the prefix in this driver is accel/rocket:, not drm/rocket:.

Thanks,
Sidong