Re: [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
On Thu, May 02, 2024 at 08:38:10PM +0200, 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
Reviewed-by: Liviu Dudau
> ---
> 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) {
> -
Re: [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
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
It makes the code more readable too - I like it.
Reviewed-by: Steven Price
> ---
> 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);
>
>
[PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
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
---
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)
