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) <[email protected]> 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 = ðosu_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/[email protected]?part=4
