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

[Christian König]

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@xxxxxxxxx. 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@xxxxxxxxx>
>> ---
>>   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
>>
>