Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
Pre-existing issues: - [High] Crash due to unconditional register access in IRQ handler while clocks are disabled - [High] Null pointer dereference due to calling drm_sched_job_cleanup() on an uninitialized structure - [High] Permanent memory leak of done_fence on job submission error paths - [High] Use-After-Free of DRM scheduler due to missing drm_dev_enter() protection - [High] Asymmetric teardown allows device interrupts to trigger a Use-After-Free on the destroyed DRM scheduler - [High] Race condition leading to Use-After-Free of job and done_fence between IRQ handler and timeout worker - [High] Missing read-back / flush after clearing the interrupt via writel_relaxed causes an IRQ storm -- commit 0bdd78d5a0b025f21b3dfd162dec6e0042165c19 Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Drop IRQF_SHARED flag The IRQF_SHARED flag doesn't work with runtime-pm as the IRQ handler could run without resuming the device. This could also be fixed with runtime-pm calls in the IRQ handler, but there is no known need for a shared IRQ. Cc: [email protected] > diff --git a/drivers/accel/ethosu/ethosu_job.c > b/drivers/accel/ethosu/ethosu_job.c > index 99c8812691e55..c33f2877b3850 100644 > --- a/drivers/accel/ethosu/ethosu_job.c > +++ b/drivers/accel/ethosu/ethosu_job.c > @@ -337,7 +337,7 @@ int ethosu_job_init(struct ethosu_device *edev) > ret = devm_request_threaded_irq(dev, edev->irq, > ethosu_job_irq_handler, > ethosu_job_irq_handler_thread, > - IRQF_SHARED, KBUILD_MODNAME, > + 0, KBUILD_MODNAME, > edev); [Severity: High] This is a pre-existing issue, but I see the commit message says "This could also be fixed with runtime-pm calls in the IRQ handler". Does this change completely address the issue with clocks being disabled when the IRQ handler runs? For instance, in ethosu_probe(), the hardware IRQ is requested via ethosu_job_init() before the device is resumed and clocks are enabled in ethosu_init(): drivers/accel/ethosu/ethosu_drv.c:ethosu_probe() { ... ret = ethosu_job_init(ethosudev); if (ret) return ret; ret = ethosu_init(ethosudev); ... } If a pending interrupt fires immediately, ethosu_job_irq_handler() will access registers while clocks are gated, which can trigger a bus fault: drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() { ... u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS); ... } Similarly, in ethosu_job_timedout(), clocks are forcibly disabled without synchronizing or disabling the IRQ: drivers/accel/ethosu/ethosu_job.c:ethosu_job_timedout() { ... pm_runtime_force_suspend(dev->base.dev); ... } Can a concurrent or delayed interrupt still crash the system here? [Severity: High] This is a pre-existing issue, but can returning IRQ_WAKE_THREAD immediately after a relaxed write lead to an IRQ storm? drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() { ... writel_relaxed(CMD_CLEAR_IRQ, dev->regs + NPU_REG_CMD); return IRQ_WAKE_THREAD; } Since the relaxed write is posted asynchronously and the interrupt is not requested with IRQF_ONESHOT, the generic IRQ core will unmask the interrupt line immediately. If the write hasn't reached the hardware to deassert the line, won't the CPU immediately take the interrupt again? Would adding a dummy register read-back ensure the interrupt is cleared before returning? [Severity: High] This is a pre-existing issue, but is it safe to bind the IRQ lifetime to devres while manually destroying the DRM scheduler in ethosu_remove()? drivers/accel/ethosu/ethosu_drv.c:ethosu_remove() { ... ethosu_job_fini(ethosudev); ... } Since the IRQ remains active until the devres teardown phase, if an interrupt fires after ethosu_job_fini(), ethosu_job_handle_irq() could call drm_sched_fault() on the already destroyed scheduler: drivers/accel/ethosu/ethosu_job.c:ethosu_job_handle_irq() { ... drm_sched_fault(&dev->sched); ... } Could this lead to a use-after-free during device unbind? [Severity: High] This is a pre-existing issue, but can the threaded IRQ handler race with the timeout worker causing a use-after-free? The threaded IRQ handler reads dev->in_flight_job and can be preempted: drivers/accel/ethosu/ethosu_job.c:ethosu_job_handle_irq() { ... job = READ_ONCE(dev->in_flight_job); if (job) { WRITE_ONCE(dev->in_flight_job, NULL); dma_fence_signal(job->done_fence); } ... } If ethosu_job_timedout() executes concurrently, it stops the scheduler and sets dev->in_flight_job to NULL, freeing the job independently: drivers/accel/ethosu/ethosu_job.c:ethosu_job_timedout() { ... drm_sched_stop(&dev->sched, bad); ... WRITE_ONCE(dev->in_flight_job, NULL); ... } When the asynchronous IRQ handler resumes, will it call dma_fence_signal() using the stale job pointer and its nested fence? [Severity: High] This is a pre-existing issue, but can a failed allocation in ethosu_ioctl_submit_job() lead to a NULL pointer dereference? drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit_job() { ... ejob->done_fence = kzalloc_obj(*ejob->done_fence); if (!ejob->done_fence) { ... goto out_cleanup_job; } ... out_cleanup_job: drm_sched_job_cleanup(&ejob->base); ... } Because drm_sched_job_init() hasn't been called yet, ejob->base.s_fence is NULL. Won't drm_sched_job_cleanup() blindly dereference this pointer during the error path? [Severity: High] This is a pre-existing issue, but does ethosu_job_err_cleanup() leak the done_fence allocation? In ethosu_ioctl_submit_job(), done_fence is dynamically allocated: drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit_job() { ... ejob->done_fence = kzalloc_obj(*ejob->done_fence); ... } If validation fails later (e.g., invalid BO handles), the code falls through to ethosu_job_err_cleanup(), which frees the job object but doesn't appear to free done_fence: drivers/accel/ethosu/ethosu_job.c:ethosu_job_err_cleanup() { ... kfree(job); } Could this lead to a memory leak when users submit invalid jobs? [Severity: High] This is a pre-existing issue, but is the ETHOSU_SUBMIT ioctl handler missing drm_dev_enter() / drm_dev_exit() protection? drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit() { int ethosu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file) ... } If the device is unbound (e.g., via sysfs or hot-unplug), ethosu_remove() tears down the DRM scheduler: drivers/accel/ethosu/ethosu_drv.c:ethosu_remove() { ... ethosu_job_fini(ethosudev); ... } If a concurrent job submission from a process holding the DRM fd continues executing, could it access the destroyed scheduler and cause a use-after-free? > if (ret) { > dev_err(dev, "failed to request irq\n"); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
