Re: [PATCH] accel/rocket: number the cores by devicetree position, not bind order

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Global `rdev` pointer is left containing an `ERR_PTR` if `rocket_device_init()` fails, leading to a kernel panic on subsequent probes.
- [High] Unbinding a core (other than the last one) breaks `rdev->cores` array tracking, leading to resource leaks, missing remaining cores during lookup, and overwriting active cores on rebind.
- [High] Memory allocated via `devm_*` on `drm_dev->dev` in `rocket_device_init()` permanently leaks when all cores are unbound.
- [High] Global static variable `rdev` and its state (`rdev->num_cores`) are accessed and initialized without synchronization, leading to TOCTOU races during concurrent probing.
- [High] Premature `drm_dev_register()` inside `rocket_device_init()` exposes the device node to userspace before core initialization completes, leading to NULL pointer dereference on open.
- [High] The `scheds` array allocated in `rocket_job_open()` leaks if there is only 1 core or if `drm_sched_entity_init()` fails.
- [High] Unhandled errors in `rocket_ioctl_submit()` silently drop user jobs, causing implicit synchronization failures.
--

commit 536d84c08923eb208c65c6e48a6e81dd4a63e6f5
Author: Igor Paunovic <royalnet026@gmail.com>

accel/rocket: number the cores by devicetree position, not bind order

rocket_job_hw_submit() programs the S_POINTER registers of a core with an
extra bit derived from core->index, the way the vendor driver derives it
from the hardware number of the core. rocket_probe() sets core->index to
the slot the core takes in rdev->cores[], which is the order the cores
bind in.

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..e2cde31443ffa 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
[ ... ]
> @@ -157,10 +157,39 @@ static const struct drm_driver rocket_drm_driver = {
[ ... ]
>  static int rocket_probe(struct platform_device *pdev)
>  {
> +	int index = rocket_core_hw_index(&pdev->dev);
>  	int ret;
>  
> +	if (index < 0)
> +		return index;
> +
>  	if (rdev == NULL) {
>  		/* First core probing, initialize DRM device. */
>  		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);

[Severity: High]
This is a pre-existing issue, but I noticed that memory allocated via devm_* 
on drm_dev->dev in rocket_device_init() might leak. 

When the last core is unbound, rocket_remove() calls rocket_device_fini() 
and sets rdev to NULL, but drm_dev is not unregistered, so the attached 
devm resources are not freed. 

Will a subsequent bind cause a permanent memory leak?

Also, another pre-existing issue: is this global rdev accessed and modified 
without synchronization? Concurrent probes might evaluate rdev == NULL and 
race to double-initialize the DRM device.

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

[Severity: High]
This is a pre-existing issue, but how does this array assignment handle 
unbinds? 

When rocket_remove() is called, it decrements rdev->num_cores without 
compacting the array:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
	...
	rocket_core_fini(&rdev->cores[core]);
	rdev->num_cores--;
	...
}

If a core other than the last one is unbound, won't this overwrite an active 
core when a new core probes and takes the slot at num_cores?

Additionally, find_core_for_dev() uses num_cores as an upper bound:

drivers/accel/rocket/rocket_drv.c:find_core_for_dev() {
	for (unsigned int core = 0; core < rdev->num_cores; core++) {
		if (dev == rdev->cores[core].dev)
			return core;
	}
	...
}

Will it fail to find remaining active cores located beyond the shrunk 
boundary?

> -	rdev->cores[core].index = core;
> +	rdev->cores[core].index = index;
>  
>  	rdev->num_cores++;

[Severity: High]
This is a pre-existing issue, but might this unprotected increment of the 
shared state lead to desynchronization and array bounds violations during 
concurrent probing?

[ ... ]

Some other pre-existing issues were noticed during review:

[Severity: High]
If rocket_device_init() fails, it returns an ERR_PTR which is left in the 
global rdev pointer:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
	...
	if (rdev == NULL) {
		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
		if (IS_ERR(rdev)) {
			...
			return PTR_ERR(rdev);
		}
	}
	
	unsigned int core = rdev->num_cores;
	...
}

Could this leave rdev holding a poisoned pointer instead of NULL? If rdev 
holds a poisoned ERR_PTR from a previous probe failure, will the subsequent 
access to rdev->num_cores dereference it and cause a kernel panic?


[Severity: High]
Does rocket_device_init() expose the device node to userspace prematurely? 

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
	...
	err = drm_dev_register(ddev, 0);
	if (err)
		return ERR_PTR(err);
	...
}

If userspace intercepts the registration and immediately invokes open(), 
could rocket_open() access an uninitialized core dev pointer?

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

Since the core properties are not assigned until later in rocket_probe(), 
will this panic when trying to allocate a domain?


[Severity: High]
Does the scheds array allocated in rocket_job_open() leak if 
rdev->num_cores == 1?

drivers/accel/rocket/rocket_job.c:rocket_job_open() {
	...
	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
							 rdev->num_cores);
	...
	ret = drm_sched_entity_init(&rocket_priv->sched_entity,
				    DRM_SCHED_PRIORITY_NORMAL,
				    scheds,
				    rdev->num_cores, NULL);
	...
}

If num_cores is 1, drm_sched_entity_init() intentionally sets 
entity->sched_list to NULL instead of storing the array. Then during close:

drivers/accel/rocket/rocket_job.c:rocket_job_close() {
	...
	kfree(entity->sched_list);
	...
}

Doesn't this incorrectly assume entity->sched_list always holds the pointer, 
causing a leak when num_cores == 1 or if drm_sched_entity_init() fails?


[Severity: High]
Are job submission errors ignored in rocket_ioctl_submit()?

drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit() {
	...
	for (i = 0; i < args->job_count; i++)
		rocket_ioctl_submit_job(dev, file, &jobs[i]);
	...
}

If rocket_ioctl_submit_job() fails, the loop blindly discards its error 
return value. Will this silently drop user jobs and cause implicit 
synchronization failures since the output fences are never injected?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905135612.7324-1-royalnet026@gmail.com?part=1