ethosu_job_init() requests the managed IRQ before initializing the DRM
scheduler. If drm_sched_init() fails, the error path calls drm_sched_fini()
on a partially initialized scheduler. The IRQ also remains registered until
devres cleanup after normal or probe-error paths destroy the scheduler.
Initialize the scheduler before requesting the IRQ. Scheduler setup failure
then needs no teardown, while IRQ setup failure can tear down a valid
scheduler. Free the IRQ explicitly before drm_sched_fini() to prevent
interrupt handlers from accessing scheduler state during teardown.
Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: [email protected]
Reported-by: Sashiko <[email protected]>
Link:
https://sashiko.dev/#/patchset/[email protected]?part=1
Signed-off-by: GuoHan Zhao <[email protected]>
---
New in v2.
Sashiko reports:
> 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?
> 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?
drivers/accel/ethosu/ethosu_job.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/accel/ethosu/ethosu_job.c
b/drivers/accel/ethosu/ethosu_job.c
index b76924645aaa..5847433e87e7 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -315,6 +315,14 @@ int ethosu_job_init(struct ethosu_device *edev)
if (edev->irq < 0)
return edev->irq;
+ edev->fence_context = dma_fence_context_alloc(1);
+
+ ret = drm_sched_init(&edev->sched, &args);
+ if (ret) {
+ dev_err(dev, "Failed to create scheduler: %d\n", ret);
+ return ret;
+ }
+
ret = devm_request_threaded_irq(dev, edev->irq,
ethosu_job_irq_handler,
ethosu_job_irq_handler_thread,
@@ -322,14 +330,6 @@ int ethosu_job_init(struct ethosu_device *edev)
edev);
if (ret) {
dev_err(dev, "failed to request irq\n");
- return ret;
- }
-
- edev->fence_context = dma_fence_context_alloc(1);
-
- ret = drm_sched_init(&edev->sched, &args);
- if (ret) {
- dev_err(dev, "Failed to create scheduler: %d\n", ret);
goto err_sched;
}
@@ -342,6 +342,7 @@ int ethosu_job_init(struct ethosu_device *edev)
void ethosu_job_fini(struct ethosu_device *dev)
{
+ devm_free_irq(dev->base.dev, dev->irq, dev);
drm_sched_fini(&dev->sched);
}
--
2.43.0
