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 <[email protected]> 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/[email protected]?part=1
