drm/amdgpu: Fix error handling in amdgpu_xcp_cfg_sysfs_init()

[PATCH] drm/amdgpu: Fix error handling in amdgpu_xcp_cfg_sysfs_init()

Posted by Guangshuo Li 1 month, 2 weeks ago

Once kobject_init_and_add() fails for an XCP resource kobject, we
should call kobject_put() to decrement the reference count for cleanup.
Otherwise, it could cause a memory leak.

The error handling loop also uses xcp_res[i] instead of xcp_res[j],
so it fails to put the previously added resource kobjects and may put
the failed kobject more than once.

Fix this by putting the failed resource kobject before jumping to the
error path, and by using the correct loop index when putting the
previously added resource kobjects.

Found by code review.

Fixes: 4ae86dc87850 ("drm/amdgpu: Add sysfs nodes to get xcp details")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
index cc5f4e01e38f..315e33a9d7c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
@@ -948,15 +948,17 @@ static void amdgpu_xcp_cfg_sysfs_init(struct amdgpu_device *adev)
 					 &xcp_cfg_res_sysfs_ktype,
 					 &xcp_cfg->kobj, "%s",
 					 xcp_res_names[rid]);
-		if (r)
+		if (r) {
+			kobject_put(&xcp_res->kobj);
 			goto err;
+		}
 	}
 
 	adev->xcp_mgr->xcp_cfg = xcp_cfg;
 	return;
 err:
 	for (j = 0; j < i; j++) {
-		xcp_res = &xcp_cfg->xcp_res[i];
+		xcp_res = &xcp_cfg->xcp_res[j];
 		kobject_put(&xcp_res->kobj);
 	}
 
-- 
2.43.0

Re: [PATCH] drm/amdgpu: Fix error handling in amdgpu_xcp_cfg_sysfs_init()

Posted by Lazar, Lijo 1 month, 2 weeks ago


On 28-Apr-26 5:15 PM, Guangshuo Li wrote:
> [You don't often get email from lgs201920130244@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Once kobject_init_and_add() fails for an XCP resource kobject, we
> should call kobject_put() to decrement the reference count for cleanup.
> Otherwise, it could cause a memory leak.
> 
> The error handling loop also uses xcp_res[i] instead of xcp_res[j],
> so it fails to put the previously added resource kobjects and may put
> the failed kobject more than once.
> 
> Fix this by putting the failed resource kobject before jumping to the
> error path, and by using the correct loop index when putting the
> previously added resource kobjects.
> 
> Found by code review.
> 
> Fixes: 4ae86dc87850 ("drm/amdgpu: Add sysfs nodes to get xcp details")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> index cc5f4e01e38f..315e33a9d7c9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> @@ -948,15 +948,17 @@ static void amdgpu_xcp_cfg_sysfs_init(struct amdgpu_device *adev)
>                                           &xcp_cfg_res_sysfs_ktype,
>                                           &xcp_cfg->kobj, "%s",
>                                           xcp_res_names[rid]);
> -               if (r)
> +               if (r) {
> +                       kobject_put(&xcp_res->kobj);
>                          goto err;
> +               }
>          }
> 
>          adev->xcp_mgr->xcp_cfg = xcp_cfg;
>          return;
>   err:
>          for (j = 0; j < i; j++) {
> -               xcp_res = &xcp_cfg->xcp_res[i];
> +               xcp_res = &xcp_cfg->xcp_res[j];

Good catch. What about just keeping it j <= i? Seeing only one path to 
get to this error handling.

Thanks,
Lijo

>                  kobject_put(&xcp_res->kobj);
>          }
> 
> --
> 2.43.0
>

Re: [PATCH] drm/amdgpu: Fix error handling in amdgpu_xcp_cfg_sysfs_init()

Posted by Christian König 1 month, 2 weeks ago

On 4/28/26 14:10, Lazar, Lijo wrote:
> 
> 
> On 28-Apr-26 5:15 PM, Guangshuo Li wrote:
>> [You don't often get email from lgs201920130244@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Once kobject_init_and_add() fails for an XCP resource kobject, we
>> should call kobject_put() to decrement the reference count for cleanup.
>> Otherwise, it could cause a memory leak.
>>
>> The error handling loop also uses xcp_res[i] instead of xcp_res[j],
>> so it fails to put the previously added resource kobjects and may put
>> the failed kobject more than once.
>>
>> Fix this by putting the failed resource kobject before jumping to the
>> error path, and by using the correct loop index when putting the
>> previously added resource kobjects.
>>
>> Found by code review.
>>
>> Fixes: 4ae86dc87850 ("drm/amdgpu: Add sysfs nodes to get xcp details")
>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> index cc5f4e01e38f..315e33a9d7c9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> @@ -948,15 +948,17 @@ static void amdgpu_xcp_cfg_sysfs_init(struct amdgpu_device *adev)
>>                                           &xcp_cfg_res_sysfs_ktype,
>>                                           &xcp_cfg->kobj, "%s",
>>                                           xcp_res_names[rid]);
>> -               if (r)
>> +               if (r) {
>> +                       kobject_put(&xcp_res->kobj);
>>                          goto err;
>> +               }
>>          }
>>
>>          adev->xcp_mgr->xcp_cfg = xcp_cfg;
>>          return;
>>   err:
>>          for (j = 0; j < i; j++) {
>> -               xcp_res = &xcp_cfg->xcp_res[i];
>> +               xcp_res = &xcp_cfg->xcp_res[j];
> 
> Good catch. What about just keeping it j <= i? Seeing only one path to get to this error handling.

The usual idiom for cleanup in loops you will find in books is:

for (i = 0; i < N; ++i);
...
	if (r)
		goto error;

....

error:
	while (i--)
		cleanup(array[i]);


The while (i--) looks counter intuitive on first glance but is actually correct.

Regards,
Christian.

> 
> Thanks,
> Lijo
> 
>>                  kobject_put(&xcp_res->kobj);
>>          }
>>
>> -- 
>> 2.43.0
>>
>