Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] This problem wasn't introduced by this patch, but... If done_fence allocation fails in ethosu_ioctl_submit_job(), the error path jumps to out_cleanup_job and calls drm_sched_job_cleanup() on an uninitialized ejob->base, causing a kernel panic. - [High] This problem wasn't introduced by this patch, but... The shared IRQ handler accesses hardware registers while the device might be runtime suspended, and the suspend callback lacks synchronize_irq(). -- --- Patch [3]: [PATCH 3/3] accel/ethosu: fix job completion fence cleanup --- commit 985100a317b693007577d3f37217a236dddcfd49 Author: GuoHan Zhao <[email protected]> accel/ethosu: fix job completion fence cleanup Fixes a leak of the uninitialized done fence and a potential refcount underflow in the job cleanup paths. Link: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/drivers/accel/ethosu/ethosu_job.c > b/drivers/accel/ethosu/ethosu_job.c > --- a/drivers/accel/ethosu/ethosu_job.c > +++ b/drivers/accel/ethosu/ethosu_job.c > @@ -152,6 +152,13 @@ static void ethosu_job_err_cleanup(struct ethosu_job > *job) > > drm_gem_object_put(job->cmd_bo); > > + if (job->done_fence) { > + if (dma_fence_was_initialized(job->done_fence)) > + dma_fence_put(job->done_fence); > + else > + dma_fence_free(job->done_fence); > + } > + > kfree(job); > } [ ... ] [Severity: High] This is a pre-existing issue, but can this cleanup path cause a NULL pointer dereference during job submission? If done_fence allocation fails in ethosu_ioctl_submit_job(), the code jumps to out_cleanup_job before drm_sched_job_init() initializes ejob->base: ethosu_ioctl_submit_job() { ejob->done_fence = kzalloc_obj(*ejob->done_fence); if (!ejob->done_fence) { ret = -ENOMEM; goto out_cleanup_job; } ... out_cleanup_job: if (ret) drm_sched_job_cleanup(&ejob->base); } This unconditionally invokes drm_sched_job_cleanup() on the uninitialized ejob->base, which will dereference job->s_fence: drm_sched_job_cleanup() { if (kref_read(&job->s_fence->finished.refcount)) { ... } Could this allow userspace to trigger a kernel panic by causing a memory allocation failure? [Severity: High] This is a pre-existing issue, but does the IRQ handler safely access hardware if the device is suspended? The shared IRQ handler accesses hardware registers unconditionally: ethosu_job_irq_handler() { struct ethosu_device *dev = data; u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS); ... } Since the IRQ line is shared, another device might trigger the IRQ while this device is in runtime suspend (clocks disabled). If the handler accesses NPU_REG_STATUS without calling pm_runtime_get_if_active() to check the PM state, could this cause a bus fault or invalid memory access? Also, in ethosu_device_suspend(), the clocks are disabled without calling synchronize_irq(): ethosu_device_suspend() { clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks); ... } Should synchronize_irq() be called here to ensure no IRQ handler is mid-flight before powering down the hardware? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
