Re: [PATCH v7 8/9] drm/panthor: Don't defer FW event processing

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

Pre-existing issues:
- [High] Possible Use-After-Free of firmware structures when group_process_tiler_oom() runs concurrently with or after panthor_fw_unplug(). This problem wasn't introduced by this patch, but remains unresolved as replacing fw_events_work with direct threaded IRQ processing still leaves tiler_oom_work queued to an unflushed workqueue.
--

--- Patch [8]: [PATCH v7 8/9] drm/panthor: Don't defer FW event processing ---
commit 99ae40464e2d4753b77cca3059716afd0d1bd396
Author: Boris Brezillon <[email protected]>

drm/panthor: Don't defer FW event processing

This commit avoids a workqueue roundtrip by processing firmware events
immediately from panthor_sched_report_fw_events() rather than deferring
them to the fw_events_work workqueue.

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 64b5fc1a0984f..7f99745ff4041 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c

[ ... ]

> @@ -4083,7 +4055,6 @@ void panthor_sched_unplug(struct panthor_device *ptdev)
>  	struct panthor_scheduler *sched = ptdev->scheduler;
>  
>  	disable_delayed_work_sync(&sched->tick_work);
> -	disable_work_sync(&sched->fw_events_work);

[Severity: High]
This is a pre-existing issue, but since we are modifying the workqueue teardown
here in panthor_sched_unplug(), is it possible for a tiler_oom_work to execute
after panthor_fw_unplug() has already run?

In panthor_device_unplug(), the sequence calls panthor_sched_unplug() which
stops the scheduler tick and disables some works, but it doesn't appear to
flush sched->heap_alloc_wq where tiler_oom_work is queued.

If panthor_fw_unplug() subsequently destroys the firmware mappings, and a
pending tiler_oom_work executes group_process_tiler_oom():

drivers/gpu/drm/panthor/panthor_sched.c:group_process_tiler_oom()
    cs_iface = panthor_fw_get_cs_iface(ptdev, csg_id, cs_id);
    heaps = panthor_vm_get_heap_pool(group->vm, false);
    heap_address = cs_iface->output->heap_address;

Could this dereference the unmapped cs_iface->output memory and cause a
use-after-free, since the async worker doesn't seem to check drm_dev_enter()?

>  	disable_work_sync(&sched->sync_upd_work);
>  
>  	mutex_lock(&sched->lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8