On Tue, Apr 30, 2024 at 01:28:52PM +0200, Boris Brezillon wrote: > ID 0 is reserved to encode 'no-tiler-heap', the heap ID range is > [1:MAX_HEAPS_PER_POOL], which we occasionally need to turn into an index > in the [0:MAX_HEAPS_PER_POOL-1] when we want to access the context object. > > v2: > - New patch > > Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block") > Reported-by: Eric Smith <eric.smith@xxxxxxxxxxxxx> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> > Tested-by: Eric Smith <eric.smith@xxxxxxxxxxxxx> > --- > drivers/gpu/drm/panthor/panthor_heap.c | 35 +++++++++++++++++++------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c > index 683bb94761bc..b1a7dbf25fb2 100644 > --- a/drivers/gpu/drm/panthor/panthor_heap.c > +++ b/drivers/gpu/drm/panthor/panthor_heap.c > @@ -109,7 +109,11 @@ static int panthor_heap_ctx_stride(struct panthor_device *ptdev) > > static int panthor_get_heap_ctx_offset(struct panthor_heap_pool *pool, int id) Can we make id and the return type here u32? I keep thinking about returning large negative values here and how they can end up being exploited. > { > - return panthor_heap_ctx_stride(pool->ptdev) * id; > + /* ID 0 is reserved to encode 'no-tiler-heap', the valid range > + * is [1:MAX_HEAPS_PER_POOL], which we need to turn into a > + * [0:MAX_HEAPS_PER_POOL-1] context index, hence the minus one here. > + */ > + return panthor_heap_ctx_stride(pool->ptdev) * (id - 1); > } > > static void *panthor_get_heap_ctx(struct panthor_heap_pool *pool, int id) > @@ -118,6 +122,21 @@ static void *panthor_get_heap_ctx(struct panthor_heap_pool *pool, int id) > panthor_get_heap_ctx_offset(pool, id); > } > > +static int panthor_get_heap_ctx_id(struct panthor_heap_pool *pool, > + u64 heap_ctx_gpu_va) > +{ > + u64 offset = heap_ctx_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts); > + u32 heap_idx = (u32)offset / panthor_heap_ctx_stride(pool->ptdev); > + > + if (offset > U32_MAX || heap_idx >= MAX_HEAPS_PER_POOL) > + return -EINVAL; > + > + /* ID 0 is reserved to encode 'no-tiler-heap', the valid range > + * is [1:MAX_HEAPS_PER_POOL], hence the plus one here. > + */ > + return heap_idx + 1; > +} > + > static void panthor_free_heap_chunk(struct panthor_vm *vm, > struct panthor_heap *heap, > struct panthor_heap_chunk *chunk) > @@ -364,14 +383,13 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool, > u64 heap_gpu_va, > u64 chunk_gpu_va) > { > - u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts); > - u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev); > + int heap_id = panthor_get_heap_ctx_id(pool, heap_gpu_va); I would keep heap_id here u32. Why do we need to change it? Also, I don't see how panthor_get_heap_ctx_id() can ever return negative values unless we expect MAX_HEAPS_PER_POOL to be S32_MAX at some moment. > struct panthor_heap_chunk *chunk, *tmp, *removed = NULL; > struct panthor_heap *heap; > int ret; > > - if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL) > - return -EINVAL; > + if (heap_id < 0) > + return heap_id; This can then be removed if heap_id is u32. > > down_read(&pool->lock); > heap = xa_load(&pool->xa, heap_id); > @@ -427,14 +445,13 @@ int panthor_heap_grow(struct panthor_heap_pool *pool, > u32 pending_frag_count, > u64 *new_chunk_gpu_va) > { > - u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts); > - u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev); > + int heap_id = panthor_get_heap_ctx_id(pool, heap_gpu_va); Again, keep u32 unless you have good reasons ... > struct panthor_heap_chunk *chunk; > struct panthor_heap *heap; > int ret; > > - if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL) > - return -EINVAL; > + if (heap_id < 0) > + return heap_id; ... and we will not need this. Best regards, Liviu > > down_read(&pool->lock); > heap = xa_load(&pool->xa, heap_id); > -- > 2.44.0 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯