panthor_heap_grow() allocates a new chunk with panthor_alloc_heap_chunk(), which adds the chunk at the head of heap->chunks under heap->lock, and then drops the lock. panthor_heap_grow() then assumes the head of the list is the chunk it just added, and reads its GPU VA through list_first_entry() without holding heap->lock. panthor_heap_grow() and panthor_heap_return_chunk() only take pool->lock in read mode, and tiler OOM events of different groups are processed in parallel on the unbound heap_alloc_wq. If two groups use the same heap context, they can modify the chunk list concurrently: group A OOM work group B OOM work alloc C_A, list_add() alloc C_B, list_add() list_first_entry() -> C_B group scheduled out: panthor_heap_return_chunk(C_B) list_del(), destroy BO, kfree() C_B->bo <- use-after-free Even without the free, both groups end up being handed the same chunk, and the chunk allocated by group A is never linked to the heap context. Fix this by having panthor_alloc_heap_chunk() return the GPU VA of the chunk it allocated, retrieved before the chunk is published in the list, instead of looking it up afterwards. Do the same for the initial chunks allocated by panthor_heap_create(), so the first chunk GPU VA is returned by panthor_alloc_heap_chunks() rather than retrieved from the list. Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block") Cc: stable@vger.kernel.org Assisted-by: Claude:claude-opus-5.5 Signed-off-by: Osama Abdelkader --- v2: - Always write chunk_gpu_va in panthor_alloc_heap_chunk(). - Have panthor_alloc_heap_chunks() return the first chunk GPU VA, and drop the list_first_entry() lookup in panthor_heap_create(). drivers/gpu/drm/panthor/panthor_heap.c | 39 +++++++++++++++----------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c index 99311abdf1e9..febc18b43af8 100644 --- a/drivers/gpu/drm/panthor/panthor_heap.c +++ b/drivers/gpu/drm/panthor/panthor_heap.c @@ -139,7 +139,8 @@ static void panthor_free_heap_chunk(struct panthor_heap_pool *pool, static int panthor_alloc_heap_chunk(struct panthor_heap_pool *pool, struct panthor_heap *heap, - bool initial_chunk) + bool initial_chunk, + u64 *chunk_gpu_va) { struct panthor_heap_chunk *chunk; struct panthor_heap_chunk_header *hdr; @@ -181,6 +182,14 @@ static int panthor_alloc_heap_chunk(struct panthor_heap_pool *pool, panthor_kernel_bo_vunmap(chunk->bo); + /* + * Retrieve the GPU VA before the chunk becomes visible in the heap + * chunk list: once heap->lock is released, a concurrent grow or + * return_chunk on the same heap can modify the list, and the chunk + * can be freed. + */ + *chunk_gpu_va = panthor_kernel_bo_gpuva(chunk->bo); + mutex_lock(&heap->lock); list_add(&chunk->node, &heap->chunks); heap->chunk_count++; @@ -210,13 +219,19 @@ static void panthor_free_heap_chunks(struct panthor_heap_pool *pool, static int panthor_alloc_heap_chunks(struct panthor_heap_pool *pool, struct panthor_heap *heap, - u32 chunk_count) + u32 chunk_count, + u64 *first_chunk_gpu_va) { int ret; u32 i; + /* + * Chunks are inserted at the head of the list, so the last chunk + * allocated is the first chunk of the heap. + */ for (i = 0; i < chunk_count; i++) { - ret = panthor_alloc_heap_chunk(pool, heap, true); + ret = panthor_alloc_heap_chunk(pool, heap, true, + first_chunk_gpu_va); if (ret) return ret; } @@ -280,7 +295,6 @@ int panthor_heap_create(struct panthor_heap_pool *pool, u64 *first_chunk_gpu_va) { struct panthor_heap *heap; - struct panthor_heap_chunk *first_chunk; struct panthor_vm *vm; int ret = 0; u32 id; @@ -315,15 +329,11 @@ int panthor_heap_create(struct panthor_heap_pool *pool, heap->max_chunks = max_chunks; heap->target_in_flight = target_in_flight; - ret = panthor_alloc_heap_chunks(pool, heap, initial_chunk_count); + ret = panthor_alloc_heap_chunks(pool, heap, initial_chunk_count, + first_chunk_gpu_va); if (ret) goto err_free_heap; - first_chunk = list_first_entry(&heap->chunks, - struct panthor_heap_chunk, - node); - *first_chunk_gpu_va = panthor_kernel_bo_gpuva(first_chunk->bo); - down_write(&pool->lock); /* The pool has been destroyed, we can't create a new heap. */ if (!pool->vm) { @@ -437,8 +447,8 @@ int panthor_heap_grow(struct panthor_heap_pool *pool, { u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts); u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev); - struct panthor_heap_chunk *chunk; struct panthor_heap *heap; + u64 chunk_gpu_va; int ret; if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL) @@ -473,14 +483,11 @@ int panthor_heap_grow(struct panthor_heap_pool *pool, * further jobs in this queue fail immediately instead of having to * wait for the job timeout. */ - ret = panthor_alloc_heap_chunk(pool, heap, false); + ret = panthor_alloc_heap_chunk(pool, heap, false, &chunk_gpu_va); if (ret) goto out_unlock; - chunk = list_first_entry(&heap->chunks, - struct panthor_heap_chunk, - node); - *new_chunk_gpu_va = (panthor_kernel_bo_gpuva(chunk->bo) & GENMASK_ULL(63, 12)) | + *new_chunk_gpu_va = (chunk_gpu_va & GENMASK_ULL(63, 12)) | (heap->chunk_size >> 12); ret = 0; -- 2.53.0