[PATCH v3 06/09] drm/amdgpu: Few optimization and fixes for userq fence driver

[Kees Bakker]

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.PaneerSelvam at amd.com>
> Reviewed-by: Christian König <christian.koenig at 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)
> [...]