Re: [PATCH 3/3] accel/ethosu: fix job completion fence cleanup

[sashiko-bot@kernel.org]

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 <zhaoguohan@kylinos.cn>

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/20260716065219.931088-1-zhaoguohan@kylinos.cn?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/20260716082530.1023538-1-zhaoguohan@kylinos.cn?part=3