Re: [PATCH 1/3] drm/amdgpu: svm check hmm range kzalloc return NULL

[Philip Yang <yangp@amd.com>]

On 2025-10-02 17:48, Chen, Xiaogang wrote:
>
> On 10/2/2025 12:43 PM, Philip Yang wrote:
>> Add hmm_range kzalloc return NULL error check. In case the get_pages
>> return failed, free and set hmm_range to NULL, to avoid double free in
>> get_pages_done.
>>
>> Fixes: 29e6f5716115 ("drm/amdgpu: use user provided hmm_range buffer 
>> in amdgpu_ttm_tt_get_user_pages")
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> index 8c3787b00f36..e8a15751c125 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> @@ -1736,15 +1736,20 @@ static int svm_range_validate_and_map(struct 
>> mm_struct *mm,
>>                   continue;
>>               }
>>   -            WRITE_ONCE(p->svms.faulting_task, current);
>>               hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
>> -            r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, 
>> npages,
>> -                               readonly, owner,
>> -                               hmm_range);
>> -            WRITE_ONCE(p->svms.faulting_task, NULL);
>> -            if (r) {
>> -                kfree(hmm_range);
>> -                pr_debug("failed %d to get svm range pages\n", r);
>> +            if (unlikely(!hmm_range)) {
>> +                r = -ENOMEM;
>> +            } else {
>> +                WRITE_ONCE(p->svms.faulting_task, current);
>> +                r = amdgpu_hmm_range_get_pages(&prange->notifier, 
>> addr, npages,
>> +                                   readonly, owner,
>> +                                   hmm_range);
>> +                WRITE_ONCE(p->svms.faulting_task, NULL);
>> +                if (r) {
>> +                    kfree(hmm_range);
>> +                    hmm_range = NULL;
>
> How it avoid double free hmm_range? Currently hmm_range got freed at 
> amdgpu_hmm_range_get_pages_done. You free it here, then 
> amdgpu_hmm_range_get_pages_done would not free it. if do not free 
> here  amdgpu_hmm_range_get_pages_done would do.
>
> And besides free hmm_range, we also need to free hmm_range->hmm_pfns 
> that is done at amdgpu_hmm_range_get_pages_done.

if amdgpu_hmm_range_get_pages returns failed, hmm_range->hmm_pfns is 
already freed. If only free hmm_range, but don't set hmm_range to NULL, 
then amdgpu_hmm_range_get_pages_done is called and will double free 
hmm_range again.

>
> I think the real problem is hmn_range is allocated and free at 
> different places. I do not know why.

This is the change in 29e6f5716115, with details in the patch.

Regards,

Philip

>
> Regards
>
> Xiaogang
>
>
>> +                    pr_debug("failed %d to get svm range pages\n", r);
>> +                }
>>               }
>>           } else {
>>               r = -EFAULT;