Re: [PATCH v2 04/19] accel: ethosu: Quiesce jobs before scheduler teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect teardown ordering and missing disable_irq() allow the IRQ handler to re-arm the TDR timeout work after it was cancelled, leading to a Use-After-Free of scheduler runqueues.
- [High] drm_sched_entity_destroy() accesses freed scheduler runqueues when the device is unbound while DRM files remain open.
--

commit 02f285145ac0684a0a3a6e4b7752353026bebfae
Author: Rob Herring (Arm) <robh@kernel.org>
accel: ethosu: Quiesce jobs before scheduler teardown

This commit aims to fix a teardown race by stopping scheduler submission,
cancelling the timeout work, resetting the NPU, and synchronizing its IRQ
before destroying the scheduler. It also adds a cancel_job callback so
queued jobs are properly signalled with -ECANCELED.

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index ec65305e0cd7d..74a1136635f87 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c

[ ... ]

> @@ -363,6 +374,15 @@ int ethosu_job_init(struct ethosu_device *edev)
>  
>  void ethosu_job_fini(struct ethosu_device *dev)
>  {
> +	drm_sched_wqueue_stop(&dev->sched);
> +	cancel_delayed_work_sync(&dev->sched.work_tdr);
> +
> +	if (READ_ONCE(dev->in_flight_job)) {
> +		WRITE_ONCE(dev->in_flight_job, NULL);
> +		ethosu_device_reset(dev);
> +	}
> +
> +	synchronize_irq(dev->irq);

[Severity: High]
This is a pre-existing issue, but does using synchronize_irq() instead of
disable_irq() leave a race window open here?

If a hardware error IRQ fires concurrently, the IRQ thread could read the
error status before the device is reset but get preempted. Because the IRQ
is only synchronized and not disabled, the thread could resume, see the
error, and call drm_sched_fault(&dev->sched). This would re-arm
dev->sched.work_tdr after it was just cancelled.

When drm_sched_fini() is called immediately after this, it frees the
runqueues and then calls its own cancel_delayed_work_sync(&sched->work_tdr).
A re-armed work_tdr could execute concurrently in this window and access the
freed runqueues.

Should this use disable_irq() instead of synchronize_irq() to ensure the
timeout work cannot be re-armed during teardown?

[Severity: High]
This isn't a bug introduced by this patch, but does calling
ethosu_job_fini() directly from ethosu_remove() cause a use-after-free if
userspace still has the DRM file descriptor open?

If the driver is unbound (e.g., via sysfs rmmod/unbind) while userspace
holds an open file descriptor, ethosu_remove() will call ethosu_job_fini()
which in turn calls drm_sched_fini(), freeing the scheduler runqueues.

When the userspace process eventually closes the file descriptor,
ethosu_job_close() is called:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_close() {
    struct drm_sched_entity *entity = &ethosu_priv->sched_entity;

    drm_sched_entity_destroy(entity);
}

The drm_sched_entity_destroy() call will attempt to access the already
freed sched_rq spinlocks. Is there a way to defer the scheduler teardown to
the DRM device release callback instead of tearing it down in the remove
path?

>  	drm_sched_fini(&dev->sched);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-ethosu-fixes-v2-0-3767738756a4@kernel.org?part=4