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
