Re: [PATCH v3 08/17] drm/panthor: Split panthor_vm

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

New issues:
- [High] Premature removal of the AS from the reclaim list allows it to be re-added concurrently, leading to a Use-After-Free when the AS is subsequently freed.

Pre-existing issues:
- [High] If a VM is released concurrently with device unplug, the hardware MMU slot is left enabled and pointing to freed page tables.
--

--- Patch [8]: [PATCH v3 08/17] drm/panthor: Split panthor_vm ---
commit 7c73a792e45cba201c66a596be6469687f3cb78d
Author: Boris Brezillon <[email protected]>
Subject: drm/panthor: Split panthor_vm

This commit splits the panthor_vm object into panthor_vm and panthor_as
to manage address spaces and page table residency separately. This prepares
the cleanup logic for future unplug robustness improvements.

> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 6c48e88ad17fb..0418365529538 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c

[ ... ]

> @@ -2031,15 +2044,23 @@ void panthor_mmu_post_reset(struct panthor_device *ptdev)
>  	mutex_unlock(&ptdev->mmu->vm.lock);
>  }
>  
> -static void panthor_vm_free(struct drm_gpuvm *gpuvm)
> +static void panthor_vm_release(struct kref *kref)
>  {
> -	struct panthor_vm *vm = container_of(gpuvm, struct panthor_vm, base);
> -	struct panthor_device *ptdev = vm->ptdev;
> +	struct panthor_vm *vm = container_of(kref, struct panthor_vm, refcount);
> +	struct panthor_as *as = vm->as;
> +	struct panthor_device *ptdev = container_of(as->base.drm, struct panthor_device, base);
>  
> +	/* Make sure the page table behind this VM doesn't participate in reclaim
> +	 * after that point, since we're about to release everything anyway.
> +	 */
>  	mutex_lock(&ptdev->base.gem_lru_mutex);
> -	list_del_init(&vm->reclaim.lru_node);
> +	list_del_init(&as->reclaim.lru_node);
>  	mutex_unlock(&ptdev->base.gem_lru_mutex);
>  
> +	/* Unmap everything in case some BOs were still mapped. */
> +	drm_WARN_ON(&ptdev->base,
> +		    panthor_vm_unmap_range(vm, as->base.mm_start, as->base.mm_range));
> +

[Severity: High]
Can this cause a use-after-free if another thread modifies a shared BO's
reclaim state? 

Because the AS is removed from the reclaim list before its BO mappings are
unmapped, couldn't a concurrent call to
panthor_vm_update_bo_reclaim_lru_locked() see the empty list node and re-add
the AS to ptdev->reclaim.vms? 

Once panthor_as_free() executes later, the AS would be freed while it is
still linked in the global reclaim list.

>  	mutex_lock(&vm->heaps.lock);
>  	if (drm_WARN_ON(&ptdev->base, vm->heaps.pool))
>  		panthor_heap_pool_destroy(vm->heaps.pool);
> @@ -2060,29 +2081,26 @@ static void panthor_vm_free(struct drm_gpuvm *gpuvm)
>  	drm_sched_entity_destroy(&vm->entity);
>  	drm_sched_fini(&vm->sched);
>  
> -	mutex_lock(&vm->op_lock);
> +	mutex_lock(&vm->as->op_lock);
>  	mutex_lock(&ptdev->mmu->as.slots_lock);
> -	if (vm->as.id >= 0) {
> +	if (as->hw_slot.id >= 0) {
>  		int cookie;
>  
>  		if (drm_dev_enter(&ptdev->base, &cookie)) {
> -			panthor_mmu_as_disable(ptdev, vm->as.id, false);
> +			panthor_mmu_as_disable(ptdev, as->hw_slot.id, false);
>  			drm_dev_exit(cookie);
>  		}
>  
> -		ptdev->mmu->as.slots[vm->as.id].vm = NULL;
> -		clear_bit(vm->as.id, &ptdev->mmu->as.alloc_mask);
> -		list_del(&vm->as.lru_node);
> +		panthor_as_release_hw_slot_locked(as);
>  	}
>  	mutex_unlock(&ptdev->mmu->as.slots_lock);

[Severity: High]
This is a pre-existing issue, but what happens to the hardware MMU slot if
drm_dev_enter() fails during an unplug?

It looks like the code skips panthor_mmu_as_disable() but still calls
panthor_as_release_hw_slot_locked(as) directly below it.

If the slot tracking is cleared here, won't panthor_mmu_unplug() also skip
disabling the slot because it appears unused, leaving the GPU MMU active on
freed page tables?

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