Re: [PATCH v12 11/14] accel/rocket: select the per-core clock and reset counts from match data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Failure to clear the global `rdev` pointer on an error path leads to an error pointer dereference on subsequent probe attempts.
- [High] Removing and re-probing cores out of order will corrupt the state of active cores and leak resources due to flawed `rdev->num_cores` indexing.
- [High] Resources allocated using `devm_*` APIs on the global dummy `drm_dev` are permanently leaked when cores are unbound or when initialization fails.
- [High] The global `rdev` pointer and `rdev->num_cores` state are accessed and modified without synchronization during concurrent probes.
- [High] The DRM device is exposed to userspace before core initialization completes, leading to a NULL pointer dereference on early open().
- [High] Asynchronous tasks and IRQs are not canceled/disabled before destroying the DRM scheduler.
--

commit 699632a47b8954e3644c77059cfdba18cb1c898b
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: select the per-core clock and reset counts from match data

This patch updates the RKNN block driver to read clock and reset counts from
per-SoC match data instead of using compile-time constants. This accommodates
SoCs like the RK3576 which have different hardware requirements than the RK3588.
The arrays sizing logic is also updated to use the dynamically provided counts.

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..7ed64c131e9cb 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
> @@ -159,8 +159,22 @@ static const struct drm_driver rocket_drm_driver = {
>  
>  static int rocket_probe(struct platform_device *pdev)
>  {
> +	const struct rocket_soc_data *soc = of_device_get_match_data(&pdev->dev);
>  	int ret;
>  
> +	/*
> +	 * soc is dereferenced without a check by every one of its users, and
> +	 * rocket_core_init() below is the first of them. A device that bound
> +	 * by name rather than by compatible has no match data, so fail before
> +	 * anything is allocated rather than at the first dereference: the
> +	 * number of cores comes from a walk of matching DT nodes, and a device
> +	 * with no of_node was never counted by it.
> +	 */
> +	if (!soc) {
> +		dev_err(&pdev->dev, "no match data for this device\n");
> +		return -ENODEV;
> +	}
> +
>  	if (rdev == NULL) {
>  		/* First core probing, initialize DRM device. */
>  		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);

[Severity: High]
This isn't a bug introduced by this patch, but if rocket_device_init() fails
during the first core's probe, it returns an error pointer which is assigned
to the global rdev pointer:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
  ...
  if (IS_ERR(rdev)) {
    dev_err(&pdev->dev, "failed to initialize rocket device\n");
    return PTR_ERR(rdev);
  }
  ...
}

Could a subsequent probe attempt bypass the rdev == NULL check and dereference
the error pointer when accessing rdev->num_cores?

[Severity: High]
This is a pre-existing issue, but do devm_* resources allocated on the global
dummy drm_dev leak when cores are unbound?

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
  ...
  rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device,
                            ddev);
  ...
}

When all cores are unbound, rocket_device_fini() unregisters the DRM device
and sets the global rdev to NULL. Since the global drm_dev is never destroyed,
does this permanently leak the struct rocket_device, core arrays, and mutexes?

[Severity: High]
This is a pre-existing issue, but are the global rdev pointer and
rdev->num_cores state protected against concurrent probes?

If multiple RKNN cores are probed concurrently via sysfs bind, could multiple
threads observe rdev == NULL simultaneously, leading to multiple
initializations of the DRM device and races when modifying the cores array?

[Severity: High]
This isn't a bug introduced by this patch, but is the DRM device exposed to
userspace before core initialization completes?

rocket_device_init() registers the DRM device via drm_dev_register(), making
it visible to userspace. If userspace opens the device immediately:

drivers/accel/rocket/rocket_drv.c:rocket_open() {
  ...
  rocket_priv->domain = rocket_iommu_domain_create(rdev->cores[0].dev);
  ...
}

Could this lead to a NULL pointer dereference in iommu_paging_domain_alloc()
since rocket_probe() hasn't populated rdev->cores[0].dev yet?

[ ... ]

> @@ -176,6 +190,7 @@ static int rocket_probe(struct platform_device *pdev)
>  
>  	rdev->cores[core].rdev = rdev;
>  	rdev->cores[core].dev = &pdev->dev;
> +	rdev->cores[core].soc = soc;
>  	rdev->cores[core].index = core;
>  
>  	rdev->num_cores++;

[Severity: High]
This is a pre-existing issue, but does using rdev->num_cores as an array
index corrupt the state of active cores when devices are removed and
re-probed out of order?

If core 0 is unbound via sysfs, rocket_remove() decrements rdev->num_cores
without shifting the rdev->cores[] array:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
  ...
  rdev->num_cores--;
  ...
}

A subsequent unbind of core 1 would fail because find_core_for_dev() shrinks
its search bounds to the new num_cores. If a new core is then probed, it uses
the decremented num_cores as the index:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
  ...
  unsigned int core = rdev->num_cores;
  ...
  rdev->cores[core].rdev = rdev;
  ...
}

Would this overwrite the active state of core 1?

[Severity: High]
This is a pre-existing issue, but are asynchronous tasks and IRQs canceled
before destroying the DRM scheduler?

In rocket_job_fini(), drm_sched_fini() is called to tear down the DRM
scheduler before canceling the background work:

drivers/accel/rocket/rocket_job.c:rocket_job_fini() {
  ...
  drm_sched_fini(&core->sched);
    
  cancel_work_sync(&core->reset.work);
  destroy_workqueue(core->reset.wq);
  ...
}

If the background reset.work executes concurrently, it calls
rocket_reset_work() which issues drm_sched_start() on a scheduler that is
already destroyed. Could this lead to a use-after-free or list corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912065053.1519165-1-gahing@gahingwoo.com?part=11