Hi Igor, Thanks so much for clearly reviewing and explaining for me.
I'm still a beginner start contributing to kernel and this is very meaningful to me. I will keep go on what you suggested. Best regards, Triet On Mon, 17 Aug 2026, 16:30 Igor Paunovic, <[email protected]> wrote: > 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 >
