On 02/05/2024 19:38, Boris Brezillon wrote:
> Avoids use-after-free situations when panthor_fw_unplug() is called
> and the kernel BO was mapped to the FW VM.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
It makes the code more readable too - I like it.
Reviewed-by: Steven Price <steven.price@xxxxxxx>
> ---
> drivers/gpu/drm/panthor/panthor_fw.c | 4 ++--
> drivers/gpu/drm/panthor/panthor_gem.c | 8 +++++---
> drivers/gpu/drm/panthor/panthor_gem.h | 8 ++++++--
> drivers/gpu/drm/panthor/panthor_heap.c | 8 ++++----
> drivers/gpu/drm/panthor/panthor_sched.c | 11 +++++------
> 5 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 181395e2859a..b41685304a83 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -453,7 +453,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
>
> ret = panthor_kernel_bo_vmap(mem);
> if (ret) {
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), mem);
> + panthor_kernel_bo_destroy(mem);
> return ERR_PTR(ret);
> }
>
> @@ -1133,7 +1133,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
> panthor_fw_stop(ptdev);
>
> list_for_each_entry(section, &ptdev->fw->sections, node)
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), section->mem);
> + panthor_kernel_bo_destroy(section->mem);
>
> /* We intentionally don't call panthor_vm_idle() and let
> * panthor_mmu_unplug() release the AS we acquired with
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index d6483266d0c2..38f560864879 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -26,18 +26,18 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
>
> /**
> * panthor_kernel_bo_destroy() - Destroy a kernel buffer object
> - * @vm: The VM this BO was mapped to.
> * @bo: Kernel buffer object to destroy. If NULL or an ERR_PTR(), the destruction
> * is skipped.
> */
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> - struct panthor_kernel_bo *bo)
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
> {
> + struct panthor_vm *vm;
> int ret;
>
> if (IS_ERR_OR_NULL(bo))
> return;
>
> + vm = bo->vm;
> panthor_kernel_bo_vunmap(bo);
>
> if (drm_WARN_ON(bo->obj->dev,
> @@ -53,6 +53,7 @@ void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> drm_gem_object_put(bo->obj);
>
> out_free_bo:
> + panthor_vm_put(vm);
> kfree(bo);
> }
>
> @@ -106,6 +107,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
> if (ret)
> goto err_free_va;
>
> + kbo->vm = panthor_vm_get(vm);
> bo->exclusive_vm_root_gem = panthor_vm_root_gem(vm);
> drm_gem_object_get(bo->exclusive_vm_root_gem);
> bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
> index 3bccba394d00..e43021cf6d45 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.h
> +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> @@ -61,6 +61,11 @@ struct panthor_kernel_bo {
> */
> struct drm_gem_object *obj;
>
> + /**
> + * @vm: VM this private buffer is attached to.
> + */
> + struct panthor_vm *vm;
> +
> /**
> * @va_node: VA space allocated to this GEM.
> */
> @@ -136,7 +141,6 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
> size_t size, u32 bo_flags, u32 vm_map_flags,
> u64 gpu_va);
>
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> - struct panthor_kernel_bo *bo);
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
>
> #endif /* __PANTHOR_GEM_H__ */
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 143fa35f2e74..65921296a18c 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -127,7 +127,7 @@ static void panthor_free_heap_chunk(struct panthor_vm *vm,
> heap->chunk_count--;
> mutex_unlock(&heap->lock);
>
> - panthor_kernel_bo_destroy(vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
> kfree(chunk);
> }
>
> @@ -183,7 +183,7 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
> return 0;
>
> err_destroy_bo:
> - panthor_kernel_bo_destroy(vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
>
> err_free_chunk:
> kfree(chunk);
> @@ -391,7 +391,7 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
> mutex_unlock(&heap->lock);
>
> if (removed) {
> - panthor_kernel_bo_destroy(pool->vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
> kfree(chunk);
> ret = 0;
> } else {
> @@ -587,7 +587,7 @@ void panthor_heap_pool_destroy(struct panthor_heap_pool *pool)
> drm_WARN_ON(&pool->ptdev->base, panthor_heap_destroy_locked(pool, i));
>
> if (!IS_ERR_OR_NULL(pool->gpu_contexts))
> - panthor_kernel_bo_destroy(pool->vm, pool->gpu_contexts);
> + panthor_kernel_bo_destroy(pool->gpu_contexts);
>
> /* Reflects the fact the pool has been destroyed. */
> pool->vm = NULL;
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 1d2708c3ab0a..6ea094b00cf9 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -826,8 +826,8 @@ static void group_free_queue(struct panthor_group *group, struct panthor_queue *
>
> panthor_queue_put_syncwait_obj(queue);
>
> - panthor_kernel_bo_destroy(group->vm, queue->ringbuf);
> - panthor_kernel_bo_destroy(panthor_fw_vm(group->ptdev), queue->iface.mem);
> + panthor_kernel_bo_destroy(queue->ringbuf);
> + panthor_kernel_bo_destroy(queue->iface.mem);
>
> kfree(queue);
> }
> @@ -837,15 +837,14 @@ static void group_release_work(struct work_struct *work)
> struct panthor_group *group = container_of(work,
> struct panthor_group,
> release_work);
> - struct panthor_device *ptdev = group->ptdev;
> u32 i;
>
> for (i = 0; i < group->queue_count; i++)
> group_free_queue(group, group->queues[i]);
>
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->suspend_buf);
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->protm_suspend_buf);
> - panthor_kernel_bo_destroy(group->vm, group->syncobjs);
> + panthor_kernel_bo_destroy(group->suspend_buf);
> + panthor_kernel_bo_destroy(group->protm_suspend_buf);
> + panthor_kernel_bo_destroy(group->syncobjs);
>
> panthor_vm_put(group->vm);
> kfree(group);
