Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues: - [High] The patch introduces a guaranteed NULL pointer dereference in the interrupt handler during device teardown by caching the IOMMU group pointer. Pre-existing issues: - [High] The shared IRQ handler accesses hardware registers on a potentially suspended device and lacks `synchronize_irq()` in the runtime suspend path. - [High] The device can be left permanently powered on with clocks enabled indefinitely after driver removal if an asynchronous suspend is cancelled. - [High] Unsafe teardown ordering allows the timeout reset work to dereference a freed or NULL IOMMU group. - [High] The driver does not verify that an IOMMU group was successfully retrieved during initialization, leading to crashes if no IOMMU is present. - [High] The shared IRQ handler is managed by devres but the resources it depends on are destroyed manually in `rocket_job_fini`, leading to a Use-After-Free. - [High] Hardware state (`OPERATION_ENABLE`) is modified outside `core->job_lock` in the threaded IRQ handler, creating a critical TOCTOU race condition. - [Medium] Timeout reset logic uses `pm_runtime_put_noidle()`, which can prevent the device from entering a suspended state indefinitely. - [Medium] Memory leak of the dynamically allocated `scheds` array in `rocket_job_open` on the error path. -- --- Patch [3]: [PATCH 3/4] accel/rocket: Fix the extra iommu_group_get call in rocket_job_handle_irq --- commit a3d6f36e63b3077307646b0545b3207616881be9 Author: Chaoyi Chen <[email protected]> accel/rocket: Fix the extra iommu_group_get call in rocket_job_handle_irq This commit removes an extra iommu_group_get() call in rocket_job_handle_irq() that caused a reference count leak. It replaces the call with the cached core->iommu_group pointer to ensure the IOMMU group can be released properly during teardown. > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index 3141f210fcd1b..0fc4d220c48ce 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -355,7 +355,7 @@ static void rocket_job_handle_irq(struct rocket_core > *core) > return; > } > > - iommu_detach_group(NULL, iommu_group_get(core->dev)); > + iommu_detach_group(NULL, core->iommu_group); [Severity: High] Does this introduce a NULL pointer dereference during driver removal? During driver removal, rocket_core_fini() explicitly sets core->iommu_group to NULL before the DRM scheduler and devres-managed IRQ handler are stopped. If an in-flight job completes or a delayed IRQ thread runs in this window, rocket_job_handle_irq() will pass the NULL core->iommu_group to iommu_detach_group(), which dereferences it unconditionally and causes a kernel panic. > dma_fence_signal(core->in_flight_job->done_fence); > pm_runtime_put_autosuspend(core->dev); > core->in_flight_job = NULL; [Severity: High] This is a pre-existing issue, but does the shared IRQ handler access hardware registers on a potentially suspended device? If a shared interrupt is triggered by another device while the NPU is runtime-suspended, the driver will directly read INTERRUPT_RAW_STATUS without verifying if the device is active: drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() { ... u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS); ... } Accessing unclocked hardware registers can cause a bus fault. Additionally, rocket_device_runtime_suspend() disables clocks without calling synchronize_irq(): drivers/accel/rocket/rocket_drv.c:rocket_device_runtime_suspend() { ... clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), rdev->cores[core].clks); ... } This allows an in-flight IRQ handler to race with hardware power-down. [Severity: High] This is a pre-existing issue, but can the device be left permanently powered on with clocks enabled indefinitely after driver removal? During device removal, rocket_core_fini() calls pm_runtime_disable() which cancels any pending asynchronous suspend work: drivers/accel/rocket/rocket_core.c:rocket_core_fini() { ... pm_runtime_dont_use_autosuspend(core->dev); pm_runtime_disable(core->dev); ... } Because a synchronous suspend is not used prior to disabling runtime PM, the device can be left permanently in the RPM_ACTIVE state. [Severity: High] This is a pre-existing issue, but does unsafe teardown ordering allow the timeout reset work to dereference a freed or NULL IOMMU group? During driver removal, rocket_core_fini() frees the IOMMU group and sets it to NULL: drivers/accel/rocket/rocket_core.c:rocket_core_fini() { ... iommu_group_put(core->iommu_group); core->iommu_group = NULL; rocket_job_fini(core); ... } rocket_job_fini() then cancels the reset work. If a job timeout occurs concurrently and the reset work executes before cancellation, rocket_reset() will use the NULL or freed group: drivers/accel/rocket/rocket_job.c:rocket_reset() { ... iommu_detach_group(NULL, core->iommu_group); ... } [Severity: High] This is a pre-existing issue, but does the driver fail to verify if an IOMMU group was successfully retrieved during initialization? In rocket_core_init(), core->iommu_group is assigned without validation: drivers/accel/rocket/rocket_core.c:rocket_core_init() { ... core->iommu_group = iommu_group_get(dev); ... } If the device lacks an IOMMU, this returns NULL. When the first job is later submitted, rocket_job_run() passes this NULL group to iommu_attach_group(), causing a kernel panic: drivers/accel/rocket/rocket_job.c:rocket_job_run() { ... ret = iommu_attach_group(job->domain->domain, core->iommu_group); ... } [Severity: Medium] This is a pre-existing issue, but does the timeout reset logic cause a power leak by failing to queue an idle check? In rocket_reset(), the PM usage count is dropped using pm_runtime_put_noidle(): drivers/accel/rocket/rocket_job.c:rocket_reset() { ... pm_runtime_put_noidle(core->dev); ... } This decrements the usage counter but explicitly avoids queuing an idle check or autosuspend request. If no further jobs are submitted, the device will remain in a powered-on state indefinitely. [Severity: Medium] This is a pre-existing issue, but does this code leak the scheds array on the error path? The scheds array is dynamically allocated, but if drm_sched_entity_init() returns an error, the function returns immediately without freeing it: drivers/accel/rocket/rocket_job.c:rocket_job_open() { ... ret = drm_sched_entity_init(&rocket_priv->sched_entity, ...); if (WARN_ON(ret)) return ret; ... } [Severity: High] This is a pre-existing issue, but does manual cleanup race with the devres managed IRQ handler, leading to a use-after-free? The IRQ is managed via devres, but critical data structures are destroyed manually in rocket_job_fini() before devres teardown: 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 an interrupt fires between the manual cleanup and devres teardown, the IRQ handler thread will attempt to access destroyed scheduler state or workqueues. [Severity: High] This is a pre-existing issue, but is there a time-of-check to time-of-use race condition when modifying hardware state outside of core->job_lock? The IRQ thread disables the hardware by writing to OPERATION_ENABLE before acquiring the lock: drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() { ... rocket_pc_writel(core, OPERATION_ENABLE, 0x0); rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff); scoped_guard(mutex, &core->job_lock) ... } If a job times out, rocket_reset() can restart the scheduler and submit a new job. If the delayed IRQ thread for the timed-out job executes subsequently, it will write 0x0 to OPERATION_ENABLE unprotected, aborting the new job and falsely signaling success. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
