From: Steven Price <steven.price@arm.com>
To: "Boris Brezillon" <boris.brezillon@collabora.com>,
"Liviu Dudau" <liviu.dudau@arm.com>,
"Adrián Larumbe" <adrian.larumbe@collabora.com>
Cc: Christopher Healy <healych@amazon.com>,
dri-devel@lists.freedesktop.org, kernel@collabora.com
Subject: Re: [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
Date: Fri, 3 May 2024 10:22:02 +0100 [thread overview]
Message-ID: <fd2be068-e3a6-4fa9-8bd7-492cf2080691@arm.com> (raw)
In-Reply-To: <20240502183813.1612017-3-boris.brezillon@collabora.com>
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@collabora.com>
It makes the code more readable too - I like it.
Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> 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);
next prev parent reply other threads:[~2024-05-03 9:22 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-02 18:38 [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
2024-05-02 18:38 ` [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults Boris Brezillon
2024-05-03 9:21 ` Steven Price
2024-05-03 11:23 ` Liviu Dudau
2024-05-02 18:38 ` [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level Boris Brezillon
2024-05-03 9:22 ` Steven Price [this message]
2024-05-03 11:46 ` Liviu Dudau
2024-05-02 18:38 ` [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug Boris Brezillon
2024-05-03 9:22 ` Steven Price
2024-05-03 11:47 ` Liviu Dudau
2024-05-02 18:38 ` [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed Boris Brezillon
2024-05-03 9:22 ` Steven Price
2024-05-03 11:49 ` Liviu Dudau
2024-05-13 11:40 ` [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fd2be068-e3a6-4fa9-8bd7-492cf2080691@arm.com \
--to=steven.price@arm.com \
--cc=adrian.larumbe@collabora.com \
--cc=boris.brezillon@collabora.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=healych@amazon.com \
--cc=kernel@collabora.com \
--cc=liviu.dudau@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).