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

[Igor Paunovic <royalnet026@gmail.com>]

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