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@xxxxxxxxxx/

  > 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