Re: [PATCH v3 15/17] drm/panthor: Fix the unplug logic

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unconditional memory leak of non-resident VMs (including page tables and mapped BOs) during device unplug.
- [High] Groups in the `waiting` list are skipped during scheduler unplug, leading to leaked jobs and list corruption.
- [High] The device's PM runtime usage counter is permanently elevated because the `put` operation was removed while the `get` was retained.
- [High] Kernel BOs (firmware sections) are unconditionally freed during unplug, bypassing the leak_active_resources mitigation and leading to hardware Use-After-Free.
--

--- Patch [15]: [PATCH v3 15/17] drm/panthor: Fix the unplug logic ---
commit ca8eb24c765a734575adb5e02e03ca868df2f058
Author: Boris Brezillon <[email protected]>

drm/panthor: Fix the unplug logic

This commit resets the GPU before calling the component unplug functions,
drops unnecessary pm_get/put calls around sub-component unplugs, and attempts
to safely release resources at unplug time even if open FDs exist. It also
introduces a leak_active_resources flag to purposefully leak resources if the
hardware cannot be successfully stopped.

> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> [ ... ]
> @@ -112,13 +148,6 @@ void panthor_device_unplug(struct panthor_device *ptdev)
>  	panthor_gpu_unplug(ptdev);
>  	panthor_pwr_unplug(ptdev);
>  
> -	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
> -	pm_runtime_put_sync_suspend(ptdev->base.dev);
> -
> -	/* If PM is disabled, we need to call the suspend handler manually. */
> -	if (!IS_ENABLED(CONFIG_PM))
> -		panthor_device_suspend(ptdev->base.dev);
> -
>  	/* Report the unplug operation as done to unblock concurrent
>  	 * panthor_device_unplug() callers.
>  	 */

[Severity: High]
Does this leave the PM runtime usage counter unbalanced?

The panthor_device_unplug() function retains the call to
pm_runtime_get_sync(ptdev->base.dev) earlier in this path, but this patch
removes the matching pm_runtime_put_sync_suspend() call here.

Could this permanent elevation of the usage counter prevent the device and
its parent power domains from entering low-power states?

> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> [ ... ]
> @@ -1285,11 +1285,9 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
>  
>  	disable_delayed_work_sync(&ptdev->fw->watchdog.ping_work);
>  
> -	if (!IS_ENABLED(CONFIG_PM) || pm_runtime_active(ptdev->base.dev)) {
> -		/* Make sure the IRQ handler cannot be called after that point. */
> +	/* Make sure the IRQ handler cannot be called after that point. */
> +	if (!IS_ENABLED(CONFIG_PM) || pm_runtime_active(ptdev->base.dev))
>  		panthor_job_irq_suspend(&ptdev->fw->irq);
> -		panthor_fw_stop(ptdev);
> -	}
>  
>  	list_for_each_entry(section, &ptdev->fw->sections, node)
>  		panthor_kernel_bo_destroy(section->mem);

[Severity: High]
Should this check ptdev->unplug.leak_active_resources before destroying
the kernel buffer objects?

The commit message mentions leaking resources if the hardware cannot be
stopped, but panthor_fw_unplug() unconditionally destroys the firmware
sections here.

If the hardware soft-reset failed, destroying these buffer objects could
return physical memory to the OS allocator while the active GPU still
retains access, potentially causing memory corruption.

> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> [ ... ]
> @@ -2227,6 +2238,11 @@ static bool vm_prep_for_cleanup(struct panthor_vm *vm)
>  	}
>  
>  	if (!drm_dev_enter(&ptdev->base, &cookie)) {
> +		/* Device is gone, take the unplug lock to make sure
> +		 * panthor_device_stop_before_unplug() has run and
> +		 * ::leak_active_resources is valid.
> +		 */
> +		guard(mutex)(&ptdev->unplug.lock);
>  		guard(mutex)(&ptdev->mmu->as.slots_lock);
>  
>  		/* If we're still on slot after an unplug, it means
>  		 * drm_dev_unplug() has returned but the part in
>  		 * panthor_mmu_unplug() that evicts all resident AS has
>  		 * not been executed yet. In that case, we simply queue
>  		 * the VM to the cleanup list and wait for
>  		 * panthor_mmu_unplug() to do its job.
>  		 */
>  		if (as->hw_slot.id >= 0) {
>  			list_add_tail(&vm->node, &ptdev->mmu->as.cleanup_list);
>  			return false;
>  		}
>  
>  		return true;
>  	}

[Severity: High]
When drm_dev_enter() fails during an unplug, this path returns true for
non-resident VMs without clearing the PANTHOR_AS_FORBID_UNMAP restriction.

Since panthor_mmu_freeze_before_unplug() applies this restriction to all
VMs earlier in the unplug sequence, leaving it set forces unmap operations
to fail or skip.

Could this cause an unconditional memory leak of the non-resident VMs,
their page tables, and mapped buffer objects?

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> [ ... ]
> @@ -4081,23 +4105,76 @@ void panthor_job_update_resvs(struct drm_exec *exec, struct drm_sched_job *sched
>  void panthor_sched_unplug(struct panthor_device *ptdev)
>  {
>  	struct panthor_scheduler *sched = ptdev->scheduler;
> +	struct panthor_group *group, *tmp_group;
> +	LIST_HEAD(groups);
>  
>  	disable_delayed_work_sync(&sched->tick_work);
>  	disable_work_sync(&sched->fw_events_work);
>  	disable_work_sync(&sched->sync_upd_work);
>  
>  	mutex_lock(&sched->lock);
> [ ... ]
> +	/* Now take care of the non-resident groups. */
> +	for (u32 i = 0; i < ARRAY_SIZE(sched->groups.runnable); i++)
> +		list_splice_init(&sched->groups.runnable[i], &groups);
> +
> +	for (u32 i = 0; i < ARRAY_SIZE(sched->groups.idle); i++)
> +		list_splice_init(&sched->groups.idle[i], &groups);
> +
> +	list_for_each_entry_safe(group, tmp_group, &groups, run_node) {
> +		list_del_init(&group->run_node);
> +		list_del_init(&group->wait_node);
> +		group_queue_work(group, term);
> +	}

[Severity: High]
Does this skip the groups currently waiting on synchronization objects?

The logic splices the sched->groups.runnable and sched->groups.idle
lists to terminate them, but it appears to entirely omit the
sched->groups.waiting list.

It seems any groups in the waiting list at the time of unplug will be
stranded here, leading to a memory leak and a later WARN_ON during
scheduler teardown.

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