[PATCH v2 0/2] drm/amdgpu: Fixes to amdgpu_userq_fence_driver_alloc()

[PATCH v2 0/2] drm/amdgpu: Fixes to amdgpu_userq_fence_driver_alloc()

[Dan Carpenter]

This fixes a double free on one error path in
amdgpu_userq_fence_driver_alloc() and cleans up another error path.

v2:
Did some additional cleanup

Dan Carpenter (2):
  drm/amdgpu: Fix double free in amdgpu_userq_fence_driver_alloc()
  drm/amdgpu: Clean up error handling in
    amdgpu_userq_fence_driver_alloc()

 drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

-- 
2.47.2

[PATCH v2 1/2] drm/amdgpu: Fix double free in amdgpu_userq_fence_driver_alloc()

[Dan Carpenter]

The goto frees "fence_drv" so this is a double free bug.  There is no
need to call amdgpu_seq64_free(adev, fence_drv->va) since the seq64
allocation failed so change the goto to goto free_fence_drv.  Also
propagate the error code from amdgpu_seq64_alloc() instead of hard coding
it to -ENOMEM.

Fixes: e7cf21fbb277 ("drm/amdgpu: Few optimization and fixes for userq fence driver")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
v2: No change.

 drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index a4953d668972..b012fece91e8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -84,11 +84,8 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
 	/* Acquire seq64 memory */
 	r = amdgpu_seq64_alloc(adev, &fence_drv->va, &fence_drv->gpu_addr,
 			       &fence_drv->cpu_addr);
-	if (r) {
-		kfree(fence_drv);
-		r = -ENOMEM;
-		goto free_seq64;
-	}
+	if (r)
+		goto free_fence_drv;
 
 	memset(fence_drv->cpu_addr, 0, sizeof(u64));
 
-- 
2.47.2

[PATCH v2 2/2] drm/amdgpu: Clean up error handling in amdgpu_userq_fence_driver_alloc()

[Dan Carpenter]

1) Checkpatch complains if we print an error message for kzalloc()
   failure.  The kzalloc() failure already has it's own error messages
   built in.  Also this allocation is small enough that it is guaranteed
   to succeed.
2) Return directly instead of doing a goto free_fence_drv.  The
   "fence_drv" is already NULL so no cleanup is necessary.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
v2: New patch

 drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index b012fece91e8..86eab5461162 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -75,11 +75,8 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
 	int r;
 
 	fence_drv = kzalloc(sizeof(*fence_drv), GFP_KERNEL);
-	if (!fence_drv) {
-		DRM_ERROR("Failed to allocate memory for fence driver\n");
-		r = -ENOMEM;
-		goto free_fence_drv;
-	}
+	if (!fence_drv)
+		return -ENOMEM;
 
 	/* Acquire seq64 memory */
 	r = amdgpu_seq64_alloc(adev, &fence_drv->va, &fence_drv->gpu_addr,
-- 
2.47.2

Re: [PATCH v2 1/2] drm/amdgpu: Fix double free in amdgpu_userq_fence_driver_alloc()

[Yadav, Arvind]

Reviewed-by:Reviewed-by:Arvind Yadav <arvind.yadav@amd.com>

On 4/12/2025 8:09 PM, Dan Carpenter wrote:
> The goto frees "fence_drv" so this is a double free bug.  There is no
> need to call amdgpu_seq64_free(adev, fence_drv->va) since the seq64
> allocation failed so change the goto to goto free_fence_drv.  Also
> propagate the error code from amdgpu_seq64_alloc() instead of hard coding
> it to -ENOMEM.
>
> Fixes: e7cf21fbb277 ("drm/amdgpu: Few optimization and fixes for userq fence driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> v2: No change.
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index a4953d668972..b012fece91e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -84,11 +84,8 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
>   	/* Acquire seq64 memory */
>   	r = amdgpu_seq64_alloc(adev, &fence_drv->va, &fence_drv->gpu_addr,
>   			       &fence_drv->cpu_addr);
> -	if (r) {
> -		kfree(fence_drv);
> -		r = -ENOMEM;
> -		goto free_seq64;
> -	}
> +	if (r)
> +		goto free_fence_drv;
>   
>   	memset(fence_drv->cpu_addr, 0, sizeof(u64));
>   

Re: [PATCH v2 2/2] drm/amdgpu: Clean up error handling in amdgpu_userq_fence_driver_alloc()

[Yadav, Arvind]

Reviewed-by:Arvind Yadav <arvind.yadav@amd.com>

On 4/12/2025 8:09 PM, Dan Carpenter wrote:
> 1) Checkpatch complains if we print an error message for kzalloc()
>     failure.  The kzalloc() failure already has it's own error messages
>     built in.  Also this allocation is small enough that it is guaranteed
>     to succeed.
> 2) Return directly instead of doing a goto free_fence_drv.  The
>     "fence_drv" is already NULL so no cleanup is necessary.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> v2: New patch
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index b012fece91e8..86eab5461162 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -75,11 +75,8 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
>   	int r;
>   
>   	fence_drv = kzalloc(sizeof(*fence_drv), GFP_KERNEL);
> -	if (!fence_drv) {
> -		DRM_ERROR("Failed to allocate memory for fence driver\n");
> -		r = -ENOMEM;
> -		goto free_fence_drv;
> -	}
> +	if (!fence_drv)
> +		return -ENOMEM;
>   
>   	/* Acquire seq64 memory */
>   	r = amdgpu_seq64_alloc(adev, &fence_drv->va, &fence_drv->gpu_addr,

Re: [PATCH v2 2/2] drm/amdgpu: Clean up error handling in amdgpu_userq_fence_driver_alloc()

[Alex Deucher]

Applied the series.  Thanks!

On Mon, Apr 14, 2025 at 12:48 AM Yadav, Arvind <arvyadav@amd.com> wrote:
>
> Reviewed-by:Arvind Yadav <arvind.yadav@amd.com>
>
> On 4/12/2025 8:09 PM, Dan Carpenter wrote:
> > 1) Checkpatch complains if we print an error message for kzalloc()
> >     failure.  The kzalloc() failure already has it's own error messages
> >     built in.  Also this allocation is small enough that it is guaranteed
> >     to succeed.
> > 2) Return directly instead of doing a goto free_fence_drv.  The
> >     "fence_drv" is already NULL so no cleanup is necessary.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > v2: New patch
> >
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 7 ++-----
> >   1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> > index b012fece91e8..86eab5461162 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> > @@ -75,11 +75,8 @@ int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
> >       int r;
> >
> >       fence_drv = kzalloc(sizeof(*fence_drv), GFP_KERNEL);
> > -     if (!fence_drv) {
> > -             DRM_ERROR("Failed to allocate memory for fence driver\n");
> > -             r = -ENOMEM;
> > -             goto free_fence_drv;
> > -     }
> > +     if (!fence_drv)
> > +             return -ENOMEM;
> >
> >       /* Acquire seq64 memory */
> >       r = amdgpu_seq64_alloc(adev, &fence_drv->va, &fence_drv->gpu_addr,