Re: [PATCH 3/4] accel/rocket: Fix the extra iommu_group_get call in rocket_job_handle_irq

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