Re: [PATCH 1/5] drm/amdgpu: Fix possible refcount leak for release of external_hw_fence

[Andrey Grodzovsky <andrey.grodzovsky@amd.com>]

On 2022-06-21 03:19, Christian König wrote:

> Am 21.06.22 um 00:02 schrieb Andrey Grodzovsky:
>> Problem:
>> In amdgpu_job_submit_direct - The refcount should drop by 2
>> but it drops only by 1.
>>
>> amdgpu_ib_sched->emit -> refcount 1 from first fence init
>> dma_fence_get -> refcount 2
>> dme_fence_put -> refcount 1
>>
>> Fix:
>> Add put for external_hw_fence in amdgpu_job_free/free_cb
>
> Well what is the external_hw_fence good for in this construct?


As far as I understand for direct submissions you don't want to pass a job
pointer to ib_schedule and so u can't use the embedded fence for this case.

Andrey


>
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> index 10aa073600d4..58568fdde2d0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> @@ -152,8 +152,10 @@ static void amdgpu_job_free_cb(struct 
>> drm_sched_job *s_job)
>>       /* only put the hw fence if has embedded fence */
>>       if (job->hw_fence.ops != NULL)
>>           dma_fence_put(&job->hw_fence);
>> -    else
>> +    else {
>
> When one side of the if uses {} the other side should use {} as well, 
> e.g. use } else { here.
>
> Christian.
>
>> + dma_fence_put(job->external_hw_fence);
>>           kfree(job);
>> +    }
>>   }
>>     void amdgpu_job_free(struct amdgpu_job *job)
>> @@ -165,8 +167,10 @@ void amdgpu_job_free(struct amdgpu_job *job)
>>       /* only put the hw fence if has embedded fence */
>>       if (job->hw_fence.ops != NULL)
>>           dma_fence_put(&job->hw_fence);
>> -    else
>> +    else {
>> +        dma_fence_put(job->external_hw_fence);
>>           kfree(job);
>> +    }
>>   }
>>     int amdgpu_job_submit(struct amdgpu_job *job, struct 
>> drm_sched_entity *entity,
>