[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