Op 30-09-2024 om 13:59 schreef Arunpravin Paneer Selvam:
Few optimization and fixes for userq fence driver.
v1:(Christian):
- Remove unnecessary comments.
- In drm_exec_init call give num_bo_handles as last parameter it would
making allocation of the array more efficient
- Handle return value of __xa_store() and improve the error handling of
amdgpu_userq_fence_driver_alloc().
v2:(Christian):
- Revert userq_xa xarray init to XA_FLAGS_LOCK_IRQ.
- move the xa_unlock before the error check of the call xa_err(__xa_store())
and moved this change to a separate patch as this is adding a missing
error
handling.
- Removed the unnecessary comments.
Signed-off-by: Arunpravin Paneer Selvam <arunpravin.paneersel...@amd.com>
Reviewed-by: Christian König <christian.koe...@amd.com>
---
.../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 44 ++++++++++++-------
.../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 6 +--
.../gpu/drm/amd/include/amdgpu_userqueue.h | 2 +-
4 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index ca8f01b2bd96..56bd870ff15d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -77,7 +77,8 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device
*adev,
fence_drv = kzalloc(sizeof(*fence_drv), GFP_KERNEL);
if (!fence_drv) {
DRM_ERROR("Failed to allocate memory for fence driver\n");
- return -ENOMEM;
+ r = -ENOMEM;
+ goto free_fence_drv;
}
/* Acquire seq64 memory */
@@ -85,7 +86,8 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device
*adev,
&fence_drv->cpu_addr);
if (r) {
kfree(fence_drv);
- return -ENOMEM;
+ r = -ENOMEM;
+ goto free_seq64;
Sorry to come back to this old patch.
Can I still ask you to take a closer look? The erroneous
code is still present in today's linux-next.
You've replaced a `return -ENOMEM` by setting `r` and
jumping to `free_seq64` where the freed pointer
is used again (use-after-free). And it is doing another
`kfree` with the same pointer.
}
memset(fence_drv->cpu_addr, 0, sizeof(u64));
@@ -95,7 +97,7 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device
*adev,
spin_lock_init(&fence_drv->fence_list_lock);
fence_drv->adev = adev;
- fence_drv->uq_fence_drv_xa_ref = &userq->uq_fence_drv_xa;
+ fence_drv->fence_drv_xa_ptr = &userq->fence_drv_xa;
fence_drv->context = dma_fence_context_alloc(1);
get_task_comm(fence_drv->timeline_name, current);
@@ -107,6 +109,13 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
userq->fence_drv = fence_drv;
return 0;
+
+free_seq64:
+ amdgpu_seq64_free(adev, fence_drv->gpu_addr);
+free_fence_drv:
+ kfree(fence_drv);
+
+ return r;
}
void amdgpu_userq_fence_driver_process(struct amdgpu_userq_fence_driver *fence_drv)
[...]
