Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
Pre-existing issues: - [High] The shared IRQ handler accesses hardware registers directly without ensuring the device is actively powered, leading to potential bus faults or spurious interrupts. - [High] The runtime suspend callback powers down hardware clocks without synchronizing against in-flight shared IRQ handlers. - [High] Device removal does not synchronously suspend the hardware, leaking the active state and leaving device clocks permanently enabled. - [High] Calling `drm_sched_fini()` after `drm_sched_init()` fails causes a kernel panic due to uninitialized waitqueues. - [High] The driver ignores the return value of `dma_set_mask_and_coherent()`, risking silent DMA address truncation. - [High] The return value of `ethosu_sram_init()` is ignored, which can lead to the NPU accessing an unallocated or invalid SRAM DMA address. - [High] The devm-managed shared IRQ handler can outlive the DRM scheduler during probe failure and module removal. - [High] Driver removal does not stop active hardware jobs. - [High] The `done_fence` allocation is leaked if `ethosu_ioctl_submit_job()` encounters an error during job initialization. - [High] Calling `dma_fence_put()` on a fence that skipped initialization causes a refcount underflow warning and leaks memory. -- --- Patch [1]: [PATCH 1/3] accel/ethosu: clean up resources on probe failure --- commit 78b823f053992db797e336339fffb6f86bafc10f Author: GuoHan Zhao <[email protected]> accel/ethosu: clean up resources on probe failure ethosu_job_init() creates a scheduler before ethosu_init() and drm_dev_register(). Errors from either later step return directly, leaving the scheduler workqueue allocated. A drm_dev_register() failure also leaves the optional SRAM allocation behind. Link: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/drivers/accel/ethosu/ethosu_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index ed9c748a54ad5..d1153b15ca3ee 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c [ ... ] > @@ -353,13 +366,21 @@ static int ethosu_probe(struct platform_device *pdev) > > ret = ethosu_init(ethosudev); [Severity: High] This is a pre-existing issue, but is the return value of ethosu_sram_init() checked during ethosu_init()? If the SRAM pool allocation fails, ethosudev->npu_info.sram_size still reflects the full pool size, but ethosudev->sramphys remains zero. Can this cause the NPU to attempt access to unmapped or zero physical memory when a job utilizing SRAM is submitted? > if (ret) > - return ret; > + goto err_job_fini; > > ret = drm_dev_register(ðosudev->base, 0); [Severity: High] This is a pre-existing issue, but does this code ignore the return value of dma_set_mask_and_coherent() which is called earlier in ethosu_probe()? If the platform only supports 32-bit DMA and the call fails, the default 32-bit mask is retained. The DRM core and driver might proceed assuming 40-bit DMA is active, handing the NPU physical addresses greater than 4 GB, which could lead to silent DMA address truncation and memory corruption. > if (ret) > - pm_runtime_dont_use_autosuspend(ethosudev->base.dev); > + goto err_runtime_suspend; [ ... ] > @@ -369,9 +390,7 @@ static void ethosu_remove(struct platform_device *pdev) > > drm_dev_unregister(ðosudev->base); > ethosu_job_fini(ethosudev); [Severity: High] This is a pre-existing issue, but does ethosu_remove() fail to synchronously suspend the hardware? The driver unregisters the DRM device but never explicitly suspends the hardware (e.g., via pm_runtime_put_sync_suspend()). Devres cleanup only calls pm_runtime_disable(), which cancels pending autosuspends but does not invoke the suspend callback if the device is active. Can this leave the device hardware and clocks permanently powered on after module unload? [Severity: High] This is a pre-existing issue, but can the devm-managed shared IRQ handler outlive the DRM scheduler here? The IRQ is requested using devm_request_threaded_irq(). During ethosu_remove() (and on probe failure), the driver explicitly calls ethosu_job_fini(), which destroys the DRM scheduler. Because the IRQ is managed by devm, it remains fully active until the remove function completes. If an interrupt fires in this window, could ethosu_job_irq_handler() execute and access the destroyed dev->sched or dev->in_flight_job structures, leading to a use-after-free? [Severity: High] This is a pre-existing issue, but does device removal ensure active hardware jobs are stopped? ethosu_remove() destroys the software state but fails to halt the physical NPU hardware (e.g., via ethosu_reset() or a synchronous suspend). If an active job is executing during driver unload, will the NPU continue running and asynchronously write to memory that has already been freed? > - if (ethosudev->sram) > - gen_pool_free(ethosudev->srampool, (unsigned > long)ethosudev->sram, > - ethosudev->npu_info.sram_size); > + ethosu_sram_fini(ethosudev); > } [Severity: High] This is a pre-existing issue, but can the shared IRQ handler access hardware registers while the device is suspended? drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() { ... u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS); ... } If the Ethos-U device is suspended via runtime PM and another device on the same IRQ line triggers an interrupt, ethosu_job_irq_handler() runs unconditionally. Reading from a powered-down block typically returns 0xffffffff or triggers an external bus fault. Should the handler use pm_runtime_get_if_active() before accessing hardware registers to prevent spurious interrupts or kernel panics? [Severity: High] This is a pre-existing issue, but does the runtime suspend callback safely synchronize against in-flight shared IRQ handlers? drivers/accel/ethosu/ethosu_drv.c:ethosu_device_suspend() { ... clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks); ... } The driver requests an interrupt with IRQF_SHARED. If an interrupt from a shared device fires concurrently right before the clocks are disabled, could the irq handler be mid-execution reading registers just as the clocks are cut? Should the suspend callback call synchronize_irq() before powering down hardware clocks? [Severity: High] This is a pre-existing issue, but does the error handling path for drm_sched_init() cause a panic if it fails? drivers/accel/ethosu/ethosu_job.c:ethosu_job_init() { ... ret = drm_sched_init(&edev->sched, &args); if (ret) { dev_err(dev, "Failed to create scheduler: %d\n", ret); goto err_sched; } return 0; err_sched: drm_sched_fini(&edev->sched); return ret; } If drm_sched_init() fails, it cleans up its own state. The error path then branches to err_sched and calls drm_sched_fini() on the partially initialized scheduler. Because the waitqueue head was either destroyed or never initialized, its list pointers are NULL. Will drm_sched_fini() invoking wake_up_all(&sched->job_scheduled) cause an immediate crash? [Severity: High] This is a pre-existing issue, but does ethosu_ioctl_submit_job() leak the done_fence allocation on error paths? drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit_job() { ... ejob->done_fence = kzalloc_obj(*ejob->done_fence); ... } If a subsequent operation fails (e.g., drm_gem_object_lookup returns NULL), the code branches to out_cleanup_job and calls ethosu_job_err_cleanup(). drivers/accel/ethosu/ethosu_job.c:ethosu_job_err_cleanup() { ... drm_gem_object_put(job->cmd_bo); kfree(job); } This cleanup function drops BO references and frees the job object, but appears to forget freeing the job->done_fence pointer. Could an unprivileged user repeatedly trigger this leak by passing invalid BO handles? [Severity: High] This is a pre-existing issue, but can calling dma_fence_put() on a fence that skipped initialization cause a refcount underflow? drivers/accel/ethosu/ethosu_job.c:ethosu_job_run() { ... if (unlikely(job->base.s_fence->finished.error)) return NULL; ... } If a job is aborted before it runs, ethosu_job_run() returns NULL early and skips calling dma_fence_init() on job->done_fence. Later, the DRM scheduler destroys the job: drivers/accel/ethosu/ethosu_job.c:ethosu_job_cleanup() { ... dma_fence_put(job->done_fence); ... } Because the fence was only zero-allocated and never initialized, its refcount is 0. Will kref_put() underflow, print a warning, and abort the release, permanently leaking the memory? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
