Re: [PATCH 02/11] accel: ethosu: Drop IRQF_SHARED flag

[sashiko-bot@kernel.org]

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) <robh@kernel.org>

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: stable@vger.kernel.org

> 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/20260827-ethosu-fixes-v1-0-346f9ea8791c@kernel.org?part=2