[PATCH 0/3] accel/ethosu: fix probe and job cleanup errors

[PATCH 0/3] accel/ethosu: fix probe and job cleanup errors

[zhaoguohan]

From: GuoHan Zhao <zhaoguohan@kylinos.cn>

Fix error handling and cleanup issues in the Arm Ethos-U driver.

Sashiko reported these issues while reviewing the Ethos-U MMIO mapping fix.
The report is available at:

https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1

The first two patches unwind scheduler, runtime PM, and SRAM state when
probe or SRAM setup fails. The final patch fixes completion fence ownership
across submit failures and scheduler early exits.

GuoHan Zhao (3):
  accel/ethosu: clean up resources on probe failure
  accel/ethosu: propagate SRAM initialization errors
  accel/ethosu: fix job completion fence cleanup

 drivers/accel/ethosu/ethosu_drv.c | 39 +++++++++++++++++++++++++------
 drivers/accel/ethosu/ethosu_job.c |  8 ++++++-
 2 files changed, 39 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/3] accel/ethosu: clean up resources on probe failure

[zhaoguohan]

From: GuoHan Zhao <zhaoguohan@kylinos.cn>

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.

Unwind runtime PM, SRAM, and scheduler state in reverse order on probe
failure. Disable the clocks when runtime PM setup fails as well.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1
Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
---
Sashiko report:

> This is a pre-existing issue, but was not introduced by this patch.
>
> If ethosu_init() or drm_dev_register() fail, the function returns
> directly without calling ethosu_job_fini() or freeing the sram pool.
>
> Because the job scheduler is embedded in the devm-allocated
> ethosu_device, it will be freed when devm cleans up. However, the drm
> scheduler kthreads and timers remain active and will access the freed
> memory.
>
> Can this cause a use-after-free regression leading to a kernel panic?

 drivers/accel/ethosu/ethosu_drv.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index ed9c748a54ad..d1153b15ca3e 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -293,6 +293,14 @@ static int ethosu_sram_init(struct ethosu_device *ethosudev)
 	return 0;
 }
 
+static void ethosu_sram_fini(struct ethosu_device *ethosudev)
+{
+	if (ethosudev->sram)
+		gen_pool_free(ethosudev->srampool,
+			      (unsigned long)ethosudev->sram,
+			      ethosudev->npu_info.sram_size);
+}
+
 static int ethosu_init(struct ethosu_device *ethosudev)
 {
 	int ret;
@@ -306,7 +314,7 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 	pm_runtime_use_autosuspend(ethosudev->base.dev);
 	ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev);
 	if (ret)
-		return ret;
+		goto err_suspend;
 	pm_runtime_get_noresume(ethosudev->base.dev);
 
 	ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + NPU_REG_ID);
@@ -326,6 +334,11 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 		 ethosudev->npu_info.sram_size / 1024);
 
 	return 0;
+
+err_suspend:
+	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
+	ethosu_device_suspend(ethosudev->base.dev);
+	return ret;
 }
 
 static int ethosu_probe(struct platform_device *pdev)
@@ -353,13 +366,21 @@ static int ethosu_probe(struct platform_device *pdev)
 
 	ret = ethosu_init(ethosudev);
 	if (ret)
-		return ret;
+		goto err_job_fini;
 
 	ret = drm_dev_register(&ethosudev->base, 0);
 	if (ret)
-		pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
+		goto err_runtime_suspend;
 
 	pm_runtime_put_autosuspend(ethosudev->base.dev);
+	return 0;
+
+err_runtime_suspend:
+	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
+	pm_runtime_put_sync_suspend(ethosudev->base.dev);
+	ethosu_sram_fini(ethosudev);
+err_job_fini:
+	ethosu_job_fini(ethosudev);
 	return ret;
 }
 
@@ -369,9 +390,7 @@ static void ethosu_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(&ethosudev->base);
 	ethosu_job_fini(ethosudev);
-	if (ethosudev->sram)
-		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
-			      ethosudev->npu_info.sram_size);
+	ethosu_sram_fini(ethosudev);
 }
 
 static const struct of_device_id dt_match[] = {
-- 
2.43.0

Re: [PATCH 1/3] accel/ethosu: clean up resources on probe failure

[sashiko-bot]

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

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

[PATCH 2/3] accel/ethosu: propagate SRAM initialization errors

[zhaoguohan]

From: GuoHan Zhao <zhaoguohan@kylinos.cn>

ethosu_sram_init() returns -ENOMEM if allocation from a declared SRAM
pool fails. ethosu_init() ignores the error after recording the pool size.
Probe then registers a device with a nonzero SRAM size and a zero sramphys
address, allowing jobs to program address zero for SRAM.

Propagate the error and synchronously suspend the device before the probe
path releases the scheduler.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1
Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
---
Sashiko report:

> This is a pre-existing issue, but was not introduced by this patch.
>
> In ethosu_sram_init(), if gen_pool_dma_alloc() fails, it returns an
> error. However, ethosu_init() ignores this return value.
>
> Because npu_info.sram_size was already set to gen_pool_size(), it remains
> non-zero while sramphys remains 0.
>
> When a user submits a job requesting sram, the size check passes, and
> ethosu_job_hw_submit() configures the hardware to use physical sram
> address 0x0.
>
> Could this result in a memory corruption regression or an IOMMU fault?

 drivers/accel/ethosu/ethosu_drv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index d1153b15ca3e..479156f79bc1 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -320,7 +320,9 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 	ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + NPU_REG_ID);
 	ethosudev->npu_info.config = config = readl_relaxed(ethosudev->regs + NPU_REG_CONFIG);
 
-	ethosu_sram_init(ethosudev);
+	ret = ethosu_sram_init(ethosudev);
+	if (ret)
+		goto err_runtime_suspend;
 
 	dev_info(ethosudev->base.dev,
 		 "Ethos-U NPU, arch v%ld.%ld.%ld, rev r%ldp%ld, cmd stream ver%ld, %d MACs, %dKB SRAM\n",
@@ -335,6 +337,10 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 
 	return 0;
 
+err_runtime_suspend:
+	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
+	pm_runtime_put_sync_suspend(ethosudev->base.dev);
+	return ret;
 err_suspend:
 	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
 	ethosu_device_suspend(ethosudev->base.dev);
-- 
2.43.0

Re: [PATCH 2/3] accel/ethosu: propagate SRAM initialization errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing hardware stop and wait for active jobs in the device removal path leads to DMA Use-After-Free.
- [High] The driver registers a shared interrupt handler but fails to check the device PM status before accessing hardware, and its suspend callback lacks IRQ synchronization.
- [High] Prematurely calling `drm_sched_fini()` during device removal leads to a Use-After-Free when userspace closes open DRM file entities.
- [High] Missing `drm_dev_enter()` and `drm_dev_exit()` protection in `ethosu_ioctl_submit` allows a TOCTOU race with device removal.
- [High] Calling `drm_sched_fini()` on `drm_sched_init()` failure causes a crash on uninitialized work items.
- [Medium] The device is not synchronously suspended during driver unbind/removal, leaving its hardware clocks permanently enabled.
--

--- Patch [2]: [PATCH 2/3] accel/ethosu: propagate SRAM initialization errors ---
commit d63b5342071ce711d48106e8b0fded82dbcf48e8
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>

accel/ethosu: propagate SRAM initialization errors

This patch fixes a bug where ethosu_sram_init() errors were ignored in
ethosu_init(), leading to jobs programming physical address zero for SRAM. It
adds proper error propagation and synchronous suspension in the probe path.

Link: https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index d1153b15ca3ee..479156f79bc18 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -320,7 +320,9 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  	ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + NPU_REG_ID);
>  	ethosudev->npu_info.config = config = readl_relaxed(ethosudev->regs + NPU_REG_CONFIG);
>  
> -	ethosu_sram_init(ethosudev);
> +	ret = ethosu_sram_init(ethosudev);
> +	if (ret)
> +		goto err_runtime_suspend;
>  
>  	dev_info(ethosudev->base.dev,

[ ... ]

> @@ -389,7 +391,7 @@ static void ethosu_remove(struct platform_device *pdev)
>  {
>  	struct ethosu_device *ethosudev = dev_get_drvdata(&pdev->dev);
>  
>  	drm_dev_unregister(&ethosudev->base);
>  	ethosu_job_fini(ethosudev);

[Severity: Medium]
This is a pre-existing issue, but does this removal path lack a synchronous
suspend of the device?

Since devres cleanup will eventually call pm_runtime_disable() which clears
the state but leaves hardware powered on if it was active, will failing to
call pm_runtime_put_sync_suspend() here lead to the clocks and power domains
leaking and remaining active indefinitely?

[Severity: High]
This isn't a bug introduced by this patch, but doesn't ethosu_job_fini()
destroy the DRM scheduler via drm_sched_fini() prematurely?

If userspace still holds a DRM file descriptor open after the device is
unbound, closing the file descriptor will call ethosu_job_close() which
invokes drm_sched_entity_destroy() on the already finalized scheduler.

Can this cause a use-after-free accessing destroyed DRM scheduler structures?

>  	ethosu_sram_fini(ethosudev);
>  }

[Severity: Critical]
This is a pre-existing issue, but is the SRAM freed while the hardware might
still be active?

If ethosu_remove() is executed while the NPU is actively executing a job,
ethosu_sram_fini() will immediately free the SRAM pool via gen_pool_free().

Since there is no halt of the hardware or wait for the in-flight job to
finish, could the hardware continue performing DMA read/writes to the freed
SRAM pool, leading to a DMA use-after-free?

> 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
> @@ -230,8 +230,8 @@ static irqreturn_t ethosu_job_irq_handler(int irq, void *data)
>  {
>  	struct ethosu_device *dev = data;
>  	u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS);

[Severity: High]
This is a pre-existing issue, but this interrupt is requested with IRQF_SHARED
in ethosu_job_init(). If the device enters autosuspend and turns off its
clocks, could this unconditionally read NPU_REG_STATUS and cause a synchronous
external abort or spurious interrupt logic?

Should ethosu_job_irq_handler() use pm_runtime_get_if_active() before checking
the hardware registers?

Additionally, shouldn't ethosu_device_suspend() call synchronize_irq() to
ensure no handler is mid-flight before disabling clocks?

[ ... ]

> @@ -337,9 +337,7 @@ int ethosu_job_init(struct ethosu_device *edev)
>  	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);

[Severity: High]
This isn't a bug introduced by this patch, but should drm_sched_fini() be
called if drm_sched_init() fails?

If drm_sched_init() cleans up its internal resources upon returning an error,
would unconditionally calling drm_sched_fini() here attempt to destroy
workqueues and free resources that were never successfully initialized,
potentially leading to a panic?

[ ... ]

> @@ -485,9 +483,8 @@ int ethosu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file
>  {
>  	struct drm_ethosu_submit *args = data;
>  	int ret = 0;

[Severity: High]
This is a pre-existing issue, but doesn't this ioctl lack drm_dev_enter() and
drm_dev_exit() unplug protection?

If userspace aggressively calls the DRM_ETHOSU_SUBMIT ioctl while the device
is being unbound, can it race with ethosu_remove()?

Without drm_dev_enter(), this could push a job onto the scheduler via
drm_sched_entity_push_job() while the removal thread is simultaneously
tearing it down with drm_sched_fini(), causing a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716082530.1023538-1-zhaoguohan@kylinos.cn?part=2

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

[zhaoguohan]

From: GuoHan Zhao <zhaoguohan@kylinos.cn>

ethosu_ioctl_submit_job() allocates done_fence before validating buffer
handles. Errors after allocation call ethosu_job_err_cleanup(), which frees
the job but leaks the uninitialized fence.

A scheduler dependency error also lets ethosu_job_run() return before
dma_fence_init(). Normal cleanup then passes a zeroed refcount to
dma_fence_put().

Release done_fence in the common cleanup path and use
dma_fence_was_initialized() to distinguish initialized fences from raw
allocations.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1
Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
---
Sashiko reports:

> drivers/accel/ethosu/ethosu_job.c:ethosu_job_err_cleanup() {
>         unsigned int i;
>
>         for (i = 0; i < job->region_cnt; i++)
>                 drm_gem_object_put(job->region_bo[i]);
>
>         drm_gem_object_put(job->cmd_bo);
>
>         kfree(job);
> }
>
> This is a pre-existing issue, but was not introduced by this patch.
>
> ethosu_ioctl_submit_job() allocates done_fence. If a subsequent
> validation fails, the error path calls ethosu_job_err_cleanup().
>
> This function frees the job structure but misses freeing job->done_fence.
>
> Can an unprivileged user repeatedly call the submit ioctl with invalid
> handles to trigger a done_fence leak?

> drivers/accel/ethosu/ethosu_job.c:ethosu_job_run() {
>     ...
>         if (unlikely(job->base.s_fence->finished.error))
>                 return NULL;
>
>         dma_fence_init(fence, &ethosu_fence_ops, &dev->fence_lock,
>                        dev->fence_context, ++dev->emit_seqno);
>     ...
> }
>
> This is a pre-existing issue, but was not introduced by this patch.
>
> If job->base.s_fence->finished.error is true, the function returns early
> without calling dma_fence_init() on job->done_fence.
>
> When the job is later destroyed, ethosu_job_cleanup() unconditionally
> calls dma_fence_put() on the fence:
>
> drivers/accel/ethosu/ethosu_job.c:ethosu_job_cleanup() {
>     ...
>         dma_fence_put(job->done_fence);
>     ...
> }
>
> Since the fence is zero-initialized, will calling kref_put() on it
> violate the API and cause a refcount underflow?

 drivers/accel/ethosu/ethosu_job.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index b76924645aaa..be745e36f1fa 100644
--- 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);
 }
 
@@ -162,7 +169,6 @@ static void ethosu_job_cleanup(struct kref *ref)
 
 	pm_runtime_put_autosuspend(job->dev->base.dev);
 
-	dma_fence_put(job->done_fence);
 	dma_fence_put(job->inference_done_fence);
 
 	ethosu_job_err_cleanup(job);
-- 
2.43.0

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

[sashiko-bot]

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